Don't acquire the lock when we don't need to mutate the 'children' map.
[]FIXED=23350182
--
MOS_MIGRATED_REVID=101158691
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 dd16024..f7d4fd8 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 @@
* <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 @@
* 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));
- }
- return child;
}
+ 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;
}
/**
