From 31ff791180614c03f2727349bfb74d9ecfdb7ef7 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Mon, 19 Aug 2019 11:24:05 -0700 Subject: features: Update compiler caching (#7675) Compiler caching was using the `id()` function to refer to configuration dictionary objects. If these objects are garbage-collected, this can produce incorrect results (false positive cache hits). This change replaces `id()` with an object that keeps a reference to the config dictionary so that it is not garbage-collected. --- lib/spack/spack/compilers/__init__.py | 88 +++++++++++++++++++++++------------ lib/spack/spack/test/compilers.py | 6 +-- 2 files changed, 60 insertions(+), 34 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py index 5adfd17b1a..20f40eef09 100644 --- a/lib/spack/spack/compilers/__init__.py +++ b/lib/spack/spack/compilers/__init__.py @@ -278,7 +278,7 @@ def all_compilers(scope=None): compilers = list() for items in config: items = items['compiler'] - compilers.append(compiler_from_config_entry(items)) + compilers.append(_compiler_from_config_entry(items)) return compilers @@ -305,41 +305,69 @@ def compilers_for_arch(arch_spec, scope=None): return list(get_compilers(config, arch_spec=arch_spec)) -def compiler_from_config_entry(items): - config_id = id(items) - compiler = _compiler_cache.get(config_id, None) +class CacheReference(object): + """This acts as a hashable reference to any object (regardless of whether + the object itself is hashable) and also prevents the object from being + garbage-collected (so if two CacheReference objects are equal, they + will refer to the same object, since it will not have been gc'ed since + the creation of the first CacheReference). + """ + def __init__(self, val): + self.val = val + self.id = id(val) + + def __hash__(self): + return self.id + + def __eq__(self, other): + return isinstance(other, CacheReference) and self.id == other.id - if compiler is None: - cspec = spack.spec.CompilerSpec(items['spec']) - os = items.get('operating_system', None) - target = items.get('target', None) - if not ('paths' in items and - all(n in items['paths'] for n in _path_instance_vars)): - raise InvalidCompilerConfigurationError(cspec) +def compiler_from_dict(items): + cspec = spack.spec.CompilerSpec(items['spec']) + os = items.get('operating_system', None) + target = items.get('target', None) - cls = class_for_compiler_name(cspec.name) + if not ('paths' in items and + all(n in items['paths'] for n in _path_instance_vars)): + raise InvalidCompilerConfigurationError(cspec) - compiler_paths = [] - for c in _path_instance_vars: - compiler_path = items['paths'][c] - if compiler_path != 'None': - compiler_paths.append(compiler_path) - else: - compiler_paths.append(None) + cls = class_for_compiler_name(cspec.name) - mods = items.get('modules') - if mods == 'None': - mods = [] + compiler_paths = [] + for c in _path_instance_vars: + compiler_path = items['paths'][c] + if compiler_path != 'None': + compiler_paths.append(compiler_path) + else: + compiler_paths.append(None) - alias = items.get('alias', None) - compiler_flags = items.get('flags', {}) - environment = items.get('environment', {}) - extra_rpaths = items.get('extra_rpaths', []) + mods = items.get('modules') + if mods == 'None': + mods = [] - compiler = cls(cspec, os, target, compiler_paths, mods, alias, - environment, extra_rpaths, **compiler_flags) - _compiler_cache[id(items)] = compiler + alias = items.get('alias', None) + compiler_flags = items.get('flags', {}) + environment = items.get('environment', {}) + extra_rpaths = items.get('extra_rpaths', []) + + return cls(cspec, os, target, compiler_paths, mods, alias, + environment, extra_rpaths, **compiler_flags) + + +def _compiler_from_config_entry(items): + """Note this is intended for internal use only. To avoid re-parsing + the same config dictionary this keeps track of its location in + memory. If you provide the same dictionary twice it will return + the same Compiler object (regardless of whether the dictionary + entries have changed). + """ + config_id = CacheReference(items) + compiler = _compiler_cache.get(config_id, None) + + if compiler is None: + compiler = compiler_from_dict(items) + _compiler_cache[config_id] = compiler return compiler @@ -367,7 +395,7 @@ def get_compilers(config, cspec=None, arch_spec=None): target != 'any'): continue - compilers.append(compiler_from_config_entry(items)) + compilers.append(_compiler_from_config_entry(items)) return compilers diff --git a/lib/spack/spack/test/compilers.py b/lib/spack/spack/test/compilers.py index 00a0fab41c..f34b9ceb5a 100644 --- a/lib/spack/spack/test/compilers.py +++ b/lib/spack/spack/test/compilers.py @@ -129,7 +129,7 @@ def test_compiler_flags_from_config_are_grouped(): 'modules': None } - compiler = compilers.compiler_from_config_entry(compiler_entry) + compiler = compilers.compiler_from_dict(compiler_entry) assert any(x == '-foo-flag foo-val' for x in compiler.flags['cflags']) @@ -179,9 +179,7 @@ def flag_value(flag, spec): else: compiler_entry = copy(default_compiler_entry) compiler_entry['spec'] = spec - # Disable faulty id()-based cache (issue #7647). - compilers._compiler_cache = {} - compiler = compilers.compiler_from_config_entry(compiler_entry) + compiler = compilers.compiler_from_dict(compiler_entry) return getattr(compiler, flag) -- cgit v1.2.3-60-g2f50