diff options
author | Nathan Harmata <nharmata@google.com> | 2015-08-20 21:14:44 +0000 |
---|---|---|
committer | Dmitry Lomov <dslomov@google.com> | 2015-08-21 09:43:46 +0000 |
commit | a65fcf91eea476d8fb4828285c6544f1f750df79 (patch) | |
tree | 564c78abdef3b6e19e1c7e3d128f38d24717c08a | |
parent | d802d5ba02226618710cfd62cc1aeab67488094d (diff) |
Don't acquire the lock when we don't need to mutate the 'children' map.
[]FIXED=23350182
--
MOS_MIGRATED_REVID=101158691
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/vfs/Path.java | 45 |
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; } /** |