From c83e365c5931f6421c0d0cdcd4967010406c3622 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 21 Dec 2019 11:34:12 -0800 Subject: performance: only regenerate env views once in `spack install` `spack install` previously concretized, writes the entire environment out, regenerated views, then wrote and regenerated views again. Regenerating views is slow, so ensure that we only do that once. - [x] add an option to env.write() to skip view regeneration - [x] add a note on whether regenerate_views() shouldn't just be a separate operation -- not clear if we want to keep it as part of write to ensure consistency, or take it out to avoid performance issues. --- lib/spack/spack/cmd/install.py | 7 ++++++- lib/spack/spack/environment.py | 31 +++++++++++++++++++++++-------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/spack/spack/cmd/install.py b/lib/spack/spack/cmd/install.py index 14a93da8ab..f39522ff59 100644 --- a/lib/spack/spack/cmd/install.py +++ b/lib/spack/spack/cmd/install.py @@ -259,9 +259,14 @@ def install(parser, args, **kwargs): if not args.only_concrete: concretized_specs = env.concretize() ev.display_specs(concretized_specs) - env.write() + + # save view regeneration for later, so that we only do it + # once, as it can be slow. + env.write(regenerate_views=False) + tty.msg("Installing environment %s" % env.name) env.install_all(args) + env.regenerate_views() return else: tty.die("install requires a package argument or a spack.yaml file") diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py index 60d6120cc8..275740a142 100644 --- a/lib/spack/spack/environment.py +++ b/lib/spack/spack/environment.py @@ -1182,7 +1182,12 @@ class Environment(object): self.specs_by_hash[h] = concrete def install_all(self, args=None): - """Install all concretized specs in an environment.""" + """Install all concretized specs in an environment. + + Note: this does not regenerate the views for the environment; + that needs to be done separately with a call to write(). + + """ with spack.store.db.read_transaction(): for concretized_hash in self.concretized_order: spec = self.specs_by_hash[concretized_hash] @@ -1203,8 +1208,6 @@ class Environment(object): os.remove(build_log_link) os.symlink(spec.package.build_log_path, build_log_link) - self.regenerate_views() - def all_specs_by_hash(self): """Map of hashes to spec for all specs in this environment.""" # Note this uses dag-hashes calculated without build deps as keys, @@ -1370,10 +1373,17 @@ class Environment(object): self.concretized_order = [ old_hash_to_new.get(h, h) for h in self.concretized_order] - def write(self): + def write(self, regenerate_views=True): """Writes an in-memory environment to its location on disk. - This will also write out package files for each newly concretized spec. + Write out package files for each newly concretized spec. Also + regenerate any views associated with the environment, if + regenerate_views is True. + + Arguments: + regenerate_views (bool): regenerate views as well as + writing if True. + """ # ensure path in var/spack/environments fs.mkdirp(self.path) @@ -1481,9 +1491,14 @@ class Environment(object): with fs.write_tmp_and_move(self.manifest_path) as f: _write_yaml(self.yaml, f) - # TODO: for operations that just add to the env (install etc.) this - # could just call update_view - self.regenerate_views() + # TODO: rethink where this needs to happen along with + # writing. For some of the commands (like install, which write + # concrete specs AND regen) this might as well be a separate + # call. But, having it here makes the views consistent witht the + # concretized environment for most operations. Which is the + # special case? + if regenerate_views: + self.regenerate_views() def __enter__(self): self._previous_active = _active_environment -- cgit v1.2.3-70-g09d2