Avoid locking target set if in load phase

5 files changed, 65 insertions, 30 deletions

diff --git a/libbuild2/algorithm.cxx b/libbuild2/algorithm.cxx
index 5011b26..8ef88e4 100644
--- a/libbuild2/algorithm.cxx
+++ b/libbuild2/algorithm.cxx
@@ -332,22 +332,28 @@ namespace build2
     if (*mp != nullptr) // Might already be there.
       return **mp;
 
-    pair<target&, ulock> r (
-      t.ctx.targets.insert_locked (tt,
-                                   move (dir),
-                                   move (out),
-                                   move (n),
-                                   nullopt /* ext     */,
-                                   target_decl::implied,
-                                   trace));
-
-    assert (r.second);
-
-    target& m (r.first);
-    *mp = &m;
-    m.group = &t;
-
-    return m;
+    target* m (nullptr);
+    {
+      pair<target&, ulock> r (
+        t.ctx.targets.insert_locked (tt,
+                                     move (dir),
+                                     move (out),
+                                     move (n),
+                                     nullopt /* ext     */,
+                                     target_decl::implied,
+                                     trace));
+
+      if (r.second) // Inserted.
+      {
+        m = &r.first;
+        m->group = &t;
+      }
+    }
+
+    assert (m != nullptr);
+    *mp = m;
+
+    return *m;
   };
 
   // Return the matching rule or NULL if no match and try_match is true.
diff --git a/libbuild2/bash/rule.cxx b/libbuild2/bash/rule.cxx
index 3048d3c..db3cc3e 100644
--- a/libbuild2/bash/rule.cxx
+++ b/libbuild2/bash/rule.cxx
@@ -161,6 +161,9 @@ namespace build2
 
                 if (mt != timestamp_nonexistent)
                 {
+                  // @@ Do we actually need _locked(), isn't path_mtime()
+                  //    atomic?
+                  //
                   auto rp (t.ctx.targets.insert_locked (bash::static_type,
                                                         ap.directory (),
                                                         dir_path () /* out */,
diff --git a/libbuild2/file.cxx b/libbuild2/file.cxx
index d789d20..31db943 100644
--- a/libbuild2/file.cxx
+++ b/libbuild2/file.cxx
@@ -2880,6 +2880,9 @@ namespace build2
 
       if (!t || *t == nullptr)
       {
+        // Note: we need the lock because process_path() call below is not
+        // MT-safe.
+        //
         pair<target&, ulock> r (insert_target (trace, ctx, tt, p));
         t = &r.first;
 
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index 1806e61..7eaa3a6 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -631,7 +631,9 @@ namespace build2
   const target* target_set::
   find (const target_key& k, tracer& trace) const
   {
-    slock sl (mutex_);
+    bool load (ctx.phase == run_phase::load);
+
+    slock sl (mutex_, defer_lock); if (!load) sl.lock ();
     map_type::const_iterator i (map_.find (k));
 
     if (i == map_.end ())
@@ -650,14 +652,18 @@ namespace build2
         // Between us releasing the shared lock and acquiring unique the
         // extension could change and possibly a new target that matches the
         // key could be inserted. In this case we simply re-run find ().
+        // Naturally, can't happen during load.
         //
-        sl.unlock ();
-        ul = ulock (mutex_);
-
-        if (ext) // Someone set the extension.
+        if (!load)
         {
-          ul.unlock ();
-          return find (k, trace);
+          sl.unlock ();
+          ul = ulock (mutex_);
+
+          if (ext) // Someone set the extension.
+          {
+            ul.unlock ();
+            return find (k, trace);
+          }
         }
       }
 
@@ -691,7 +697,8 @@ namespace build2
                  string name,
                  optional<string> ext,
                  target_decl decl,
-                 tracer& trace)
+                 tracer& trace,
+                 bool need_lock)
   {
     target_key tk {&tt, &dir, &out, &name, move (ext)};
     target* t (const_cast<target*> (find (tk, trace)));
@@ -715,7 +722,9 @@ namespace build2
       // case we proceed pretty much like find() except already under the
       // exclusive lock.
       //
-      ulock ul (mutex_);
+      ulock ul (mutex_, defer_lock);
+      if (ctx.phase != run_phase::load || need_lock)
+        ul.lock ();
 
       auto p (map_.emplace (target_key {&tt, &t->dir, &t->out, &t->name, e},
                             unique_ptr<target> (t)));
@@ -728,6 +737,10 @@ namespace build2
         t->decl = decl;
         t->state.inner.target_ = t;
         t->state.outer.target_ = t;
+
+        if (ctx.phase != run_phase::load && !need_lock)
+          ul.unlock ();
+
         return pair<target&, ulock> (*t, move (ul));
       }
 
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index e6622f4..696d5d0 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -1522,7 +1522,7 @@ namespace build2
           const dir_path& out,
           const string& name) const
     {
-      slock l (mutex_);
+      slock l (mutex_, defer_lock); if (ctx.phase != run_phase::load) l.lock ();
       auto i (map_.find (target_key {&type, &dir, &out, &name, nullopt}));
       return i != map_.end () ? i->second.get () : nullptr;
     }
@@ -1536,7 +1536,12 @@ namespace build2
 
     // If the target was inserted, keep the map exclusive-locked and return
     // the lock. In this case, the target is effectively still being created
-    // since nobody can see it until the lock is released.
+    // since nobody can see it until the lock is released. Note that there
+    // is normally quite a bit of contention around this map so make sure to
+    // not hold the lock longer than absolutely necessary.
+    //
+    // If need_lock is false, then release the lock (the target insertion is
+    // indicated by the presence of the associated mutex).
     //
     pair<target&, ulock>
     insert_locked (const target_type&,
@@ -1545,8 +1550,12 @@ namespace build2
                    string name,
                    optional<string> ext,
                    target_decl,
-                   tracer&);
+                   tracer&,
+                   bool need_lock = true);
 
+    // As above but instead of the lock return an indication of whether the
+    // target was inserted.
+    //
     pair<target&, bool>
     insert (const target_type& tt,
             dir_path dir,
@@ -1562,9 +1571,10 @@ namespace build2
                              move (name),
                              move (ext),
                              decl,
-                             t));
+                             t,
+                             false));
 
-      return pair<target&, bool> (p.first, p.second.owns_lock ()); // Clang 3.7
+      return pair<target&, bool> (p.first, p.second.mutex () != nullptr);
     }
 
     // Note that the following versions always enter implied targets.