Cache target base scope lookups

4 files changed, 84 insertions, 21 deletions

diff --git a/libbuild2/context.cxx b/libbuild2/context.cxx
index 8052716..7944294 100644
--- a/libbuild2/context.cxx
+++ b/libbuild2/context.cxx
@@ -848,7 +848,7 @@ namespace build2
     }
   }
 
-  bool run_phase_mutex::
+  optional<bool> run_phase_mutex::
   relock (run_phase o, run_phase n)
   {
     // Pretty much a fused unlock/lock implementation except that we always
@@ -857,6 +857,7 @@ namespace build2
     assert (o != n);
 
     bool r;
+    bool s (true); // True switch.
 
     if (o == run_phase::load)
       lm_.unlock ();
@@ -919,6 +920,13 @@ namespace build2
     {
       if (!lm_.try_lock ())
       {
+        // If we failed to acquire the load mutex, then we know there is (or
+        // was) someone before us in the load phase. And it's impossible to
+        // switch to a different phase between our calls to try_lock() above
+        // and lock() below because of our +1 in lc_.
+        //
+        s = false;
+
         ctx_.sched.deactivate (false /* external */);
         lm_.lock ();
         ctx_.sched.activate (false /* external */);
@@ -928,7 +936,7 @@ namespace build2
       r = !fail_; // Re-query.
     }
 
-    return r;
+    return r ? optional<bool> (s) : nullopt;
   }
 
   // C++17 deprecated uncaught_exception() so use uncaught_exceptions() if
@@ -1043,7 +1051,8 @@ namespace build2
     phase_lock* pl (phase_lock_instance);
     assert (&pl->ctx == &ctx);
 
-    if (!ctx.phase_mutex.relock (old_phase, new_phase))
+    optional<bool> r (ctx.phase_mutex.relock (old_phase, new_phase));
+    if (!r)
     {
       ctx.phase_mutex.relock (new_phase, old_phase);
       throw failed ();
@@ -1052,14 +1061,31 @@ namespace build2
     pl->phase = new_phase;
 
     if (new_phase == run_phase::load) // Note: load lock is exclusive.
+    {
       ctx.load_generation++;
 
+      // Invalidate cached target base_scope values if we are switching from a
+      // non-load phase (we don't cache during load which means load->load
+      // switch doesn't have anything to invalidate).
+      //
+      // @@ This is still quite expensive on project like Boost with a large
+      //    number of files (targets) and a large number of load phase
+      //    switches (due to directory buildfiles).
+      //
+      if (*r)
+      {
+        for (const unique_ptr<target>& t: ctx.targets)
+          t->base_scope_.store (nullptr, memory_order_relaxed);
+      }
+    }
+
     //text << this_thread::get_id () << " phase switch  "
     //     << old_phase << " " << new_phase;
   }
 
 #if 0
-  // NOTE: see push/pop_phase() logic if trying to enable this.
+  // NOTE: see push/pop_phase() logic if trying to enable this. Also
+  //       the load stuff above.
   //
   phase_switch::
   phase_switch (phase_unlock&& u, phase_lock&& l)
diff --git a/libbuild2/context.hxx b/libbuild2/context.hxx
index fbdf9b8..5cc2115 100644
--- a/libbuild2/context.hxx
+++ b/libbuild2/context.hxx
@@ -28,6 +28,7 @@ namespace build2
   public:
     // Acquire a phase lock potentially blocking (unless already in the
     // desired phase) until switching to the desired phase is possible.
+    // Return false on failure.
     //
     bool
     lock (run_phase);
@@ -38,9 +39,14 @@ namespace build2
     void
     unlock (run_phase);
 
-    // Switch from one phase to another.
+    // Switch from one phase to another. Return nullopt on failure (so can be
+    // used as bool), true if switched from a different phase, and false if
+    // joined/switched to the same phase (this, for example, can be used to
+    // decide if a phase switching housekeeping is really necessary). Note:
+    // currently only implemented for the load phase (always returns true
+    // for the others).
     //
-    bool
+    optional<bool>
     relock (run_phase unlock, run_phase lock);
 
     // Statistics.
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index 92db7e9..1806e61 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -115,22 +115,27 @@ namespace build2
   }
 
   const scope& target::
-  base_scope () const
+  base_scope_impl () const
   {
     // If this target is from the src tree, use its out directory to find
     // the scope.
     //
-    return ctx.scopes.find_out (out_dir ());
-  }
+    const scope& s (ctx.scopes.find_out (out_dir ()));
 
-  const scope& target::
-  root_scope () const
-  {
-    // This is tricky to cache so we do the lookup for now.
+    // Cache unless we are in the load phase.
     //
-    const scope* r (base_scope ().root_scope ());
-    assert (r != nullptr);
-    return *r;
+    if (ctx.phase != run_phase::load)
+    {
+      const scope* e (nullptr);
+      if (!base_scope_.compare_exchange_strong (
+            e,
+            &s,
+            memory_order_release,
+            memory_order_consume))
+        assert (e == &s);
+    }
+
+    return s;
   }
 
   pair<lookup, size_t> target::
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 3d8c4ff..d3e88dd 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -380,7 +380,16 @@ namespace build2
     // Most qualified scope that contains this target.
     //
     const scope&
-    base_scope () const;
+    base_scope () const
+    {
+      if (ctx.phase != run_phase::load)
+      {
+        if (const scope* s = base_scope_.load (memory_order_consume))
+          return *s;
+      }
+
+      return base_scope_impl ();
+    }
 
     // Root scope of a project that contains this target. Note that
     // a target can be out of any (known) project root in which case
@@ -388,7 +397,10 @@ namespace build2
     // then use base_scope().root_scope() expression instead.
     //
     const scope&
-    root_scope () const;
+    root_scope () const
+    {
+      return *base_scope ().root_scope ();
+    }
 
     // Root scope of a bundle amalgamation that contains this target. The
     // same notes as to root_scope() apply.
@@ -414,6 +426,16 @@ namespace build2
       return out_dir ().sub (s.out_path ());
     }
 
+    // Implementation details (use above functions instead).
+    //
+    // Base scope cached value. Invalidated every time we switch to the load
+    // phase (which is the only phase where we may insert new scopes).
+    //
+    mutable atomic<const scope*> base_scope_ {nullptr};
+
+    const scope&
+    base_scope_impl () const;
+
     // Prerequisites.
     //
     // We use an atomic-empty semantics that allows one to "swap in" a set of
@@ -1580,10 +1602,14 @@ namespace build2
     // Note: not MT-safe so can only be used during serial execution.
     //
   public:
-    using iterator = butl::map_iterator_adapter<map_type::const_iterator>;
+    using iterator = butl::map_iterator_adapter<map_type::iterator>;
+    using const_iterator = butl::map_iterator_adapter<map_type::const_iterator>;
+
+    iterator begin () {return map_.begin ();}
+    iterator end ()   {return map_.end ();}
 
-    iterator begin () const {return map_.begin ();}
-    iterator end ()   const {return map_.end ();}
+    const_iterator begin () const {return map_.begin ();}
+    const_iterator end ()   const {return map_.end ();}
 
     size_t
     size () const {return map_.size ();}