Don't acquire the lock when we don't need to mutate the 'children' map.

[]FIXED=23350182

--
MOS_MIGRATED_REVID=101158691

1 files changed, 33 insertions, 12 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
index dd1602420f..f7d4fd8dc6 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
@@ -149,7 +149,7 @@ public class Path implements Comparable<Path>, Serializable {
    * <p>The Path object must be synchronized while children is being
    * accessed.
    */
-  private IdentityHashMap<String, Reference<Path>> children;
+  private volatile IdentityHashMap<String, Reference<Path>> children;
 
   /**
    * Create a path instance.  Should only be called by {@link #createChildPath}.
@@ -216,21 +216,42 @@ public class Path implements Comparable<Path>, Serializable {
    * if it doesn't already exist.
    */
   private Path getCachedChildPath(String childName) {
-    // Don't hold the lock for the interning operation. It increases lock contention.
+    // We get a canonical instance since 'children' is an IdentityHashMap.
     childName = StringCanonicalizer.intern(childName);
-    synchronized(this) {
-      if (children == null) {
-        // 66% of Paths have size == 1, 80% <= 2
-        children = new IdentityHashMap<>(1);
+
+    // We use double double-checked locking so that we only hold the lock when we might need to
+    // mutate the 'children' variable or the map to which it refers.
+
+    // 'children' will never become null if it's already non-null, so we only need to worry about
+    // the case where it's currently null and we race with another thread executing
+    // getCachedChildPath(<doesn't matter>) trying to set 'children' to a non-null value.
+    if (children == null) {
+      synchronized (this) {
+        if (children == null) {
+          // 66% of Paths have size == 1, 80% <= 2
+          children = new IdentityHashMap<>(1);
+        }
       }
-      Reference<Path> childRef = children.get(childName);
-      Path child;
-      if (childRef == null || (child = childRef.get()) == null) {
-        child = createChildPath(childName);
-        children.put(childName, new PathWeakReferenceForCleanup(child, REFERENCE_QUEUE));
+    }
+    Path child;
+    Reference<Path> childRef = children.get(childName);
+    // If child != null, the target of the childRef WeakReference won't concurrently get GC'd since,
+    // well, we have a reference to it!. So we only need to worry about the case where 'childName'
+    // is not already cached. There are two potential races here: (i) we race with another thread
+    // executing getCachedChildPath(someMissingChildName) and (ii) we race with
+    // PATH_CHILD_CACHE_CLEANUP_THREAD trying to remove an entry from the map for another child that
+    // was GC'd. In both cases there are concurrent modifications to the IdentityHashMap, so we use
+    // conservative synchronization.
+    if (childRef == null || (child = childRef.get()) == null) {
+      synchronized (this) {
+        childRef = children.get(childName);
+        if (childRef == null || (child = childRef.get()) == null) {
+          child = createChildPath(childName);
+          children.put(childName, new PathWeakReferenceForCleanup(child, REFERENCE_QUEUE));
+        }
       }
-      return child;
     }
+    return child;
   }
 
   /**