The double-checked locking seems unnecessary.

Every double-checked lock will be followed up by a regular lock.
Either the children don't exist, in which case we take the lock
twice, or they do exist and we take the lock once.

This makes it so we only ever take the lock once.

I don't know if this makes it faster, but it makes the code
simpler, and shouldn't make it slower.

--
PiperOrigin-RevId: 146553235
MOS_MIGRATED_REVID=146553235

1 files changed, 5 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 ee81ad9adc..cfa3520ce9 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
@@ -327,19 +327,12 @@ public class Path implements Comparable<Path>, Serializable {
       // Non-cacheable children won't show up in `children` so applyToChildren won't run for these.
       return parent.createChildPath(childName);
     }
-    // We use double-checked locking so that we only hold the lock when we might need to mutate the
-    // 'children' variable. '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 (parent.children == null) {
-      synchronized (parent) {
-        if (parent.children == null) {
-          // 66% of Paths have size == 1, 80% <= 2
-          parent.children = new IdentityHashMap<>(1);
-        }
-      }
-    }
+
     synchronized (parent) {
+      if (parent.children == null) {
+        // 66% of Paths have size == 1, 80% <= 2
+        parent.children = new IdentityHashMap<>(1);
+      }
       Reference<Path> childRef = parent.children.get(childName);
       Path child;
       if (childRef == null || (child = childRef.get()) == null) {