Remove synchronization from ArtifactFactory#resolveSourceArtifactWithAncestor.
@@ -324,8 +324,14 @@ public class ArtifactFactory implements ArtifactResolver {
* relativePath} (via {@code baseExecPath.getRelative(relativePath)} if baseExecPath is not null).
* That Artifact will have root determined by the package roots of this factory if it lives in a
* subpackage distinct from that of baseExecPath, and {@code baseRoot} otherwise.
+ *
+ * <p>Thread-safety: does only reads until the call to #createArtifactIfNotValid. That may perform
+ * mutations, but is thread-safe. There is the potential for a race in which one thread observes
+ * no matching artifact in {@link #sourceArtifactCache} initially, but when it goes to create it,
+ * does find it there, but that is a benign race.
*/
- public synchronized Artifact resolveSourceArtifactWithAncestor(
+ @ThreadSafe
+ public Artifact resolveSourceArtifactWithAncestor(
PathFragment relativePath,
PathFragment baseExecPath,
ArtifactRoot baseRoot,
@@ -451,6 +457,10 @@ public class ArtifactFactory implements ArtifactResolver {
return execRoot.getRelative(execPath);
}
+ // Thread-safety: gets from sourceArtifactCache, which can be done concurrently, and may create
+ // an artifact, which is done by #getSourceArtifact in a thread-safe manner. Only non-thread-safe
+ // call is to sourceArtifactCache#markEntryAsValid, which is synchronized on this.
+ @ThreadSafe
private Artifact createArtifactIfNotValid(Root sourceRoot, PathFragment execPath) {
if (sourceRoot == null) {
return null; // not a path that we can find...
@@ -459,7 +469,21 @@ public class ArtifactFactory implements ArtifactResolver {
if (artifact != null && sourceRoot.equals(artifact.getRoot().getRoot())) {
// Source root of existing artifact hasn't changed so we should mark corresponding entry in
// the cache as valid.
+ // TODO(janakr): markEntryAsValid looks like it should be thread-safe: revisit if contention
+ // here is still an issue.
+ synchronized (this) {
+ Artifact validArtifact = sourceArtifactCache.getArtifactIfValid(execPath);
+ if (validArtifact == null) {
+ // Wasn't previously known to be valid.
sourceArtifactCache.markEntryAsValid(execPath);
+ } else {
+ Preconditions.checkState(
+ artifact.equals(validArtifact),
+ "Mismatched artifacts: %s %s",
+ artifact,
+ validArtifact);
+ }
+ }
} else {
// Must be a new artifact or artifact in the cache is stale, so create a new one.
artifact = getSourceArtifact(execPath, sourceRoot, ArtifactOwner.NullArtifactOwner.INSTANCE);
@@ -474,7 +498,7 @@ public class ArtifactFactory implements ArtifactResolver {
* @param execPath The artifact's exec path.
*/
@VisibleForTesting // for our own unit tests only.
- synchronized boolean isDerivedArtifact(PathFragment execPath) {
+ boolean isDerivedArtifact(PathFragment execPath) {
return execPath.startsWith(derivedPathPrefix);
}
}
