Refactor cycle related skyvalues to use shared empty value Previously the cycle values (were supposed to) share a common abstract base class, however usage was inconsistent. Instead refactor to eliminate the specialized value classes, remove the abstract class, and share a common empty placeholder value, which should be useful for future/other current empty values. -- MOS_MIGRATED_REVID=105217399
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessFunction.java index fa53b06..489cc0e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessFunction.java
@@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.skyframe.EmptySkyValue; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -35,8 +36,6 @@ protected abstract String getFooterMessage(); - protected abstract SkyValue getDummyValue(); - protected abstract String elementToString(S elt); @Override @@ -53,7 +52,7 @@ // The purpose of this SkyFunction is the side effect of emitting an error message exactly // once per build per unique error. env.getListener().handle(Event.error(errorMessage.toString())); - return getDummyValue(); + return EmptySkyValue.INSTANCE; } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ChainUniquenessUtils.java similarity index 80% rename from src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessValue.java rename to src/main/java/com/google/devtools/build/lib/skyframe/ChainUniquenessUtils.java index 672dfd4..6b58cf9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AbstractChainUniquenessValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ChainUniquenessUtils.java
@@ -17,15 +17,21 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; /** * A value for ensuring that an error for a cycle/chain is reported exactly once. This is achieved * by forcing the same value key for two logically equivalent errors and letting Skyframe do its * magic. */ -class AbstractChainUniquenessValue implements SkyValue { - protected static SkyKey key(SkyFunctionName functionName, ImmutableList<? extends Object> chain) { +class ChainUniquenessUtils { + + private ChainUniquenessUtils() {} + + /** + * Create a SkyKey for {@code functionName} with a canonicalized representation of the cycle + * specified by {@code chain} as the argument. {@code chain} must be non-empty. + */ + static SkyKey key(SkyFunctionName functionName, ImmutableList<? extends Object> chain) { Preconditions.checkState(!chain.isEmpty()); return new SkyKey(functionName, canonicalize(chain)); } @@ -34,6 +40,7 @@ int minPos = 0; String minString = cycle.get(0).toString(); for (int i = 1; i < cycle.size(); i++) { + // TOOD(bazel-team): Is the toString representation stable enough? String candidateString = cycle.get(i).toString(); if (candidateString.compareTo(minString) < 0) { minPos = i;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java index 1255600..8a7c84d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java
@@ -234,7 +234,7 @@ isPathPredicate(symlinkTargetRootedPath.asPath()), symlinkChain); FileSymlinkCycleException fsce = new FileSymlinkCycleException(pathAndChain.getFirst(), pathAndChain.getSecond()); - uniquenessKey = FileSymlinkCycleUniquenessValue.key(fsce.getCycle()); + uniquenessKey = FileSymlinkCycleUniquenessFunction.key(fsce.getCycle()); fse = fsce; } else { Pair<ImmutableList<RootedPath>, ImmutableList<RootedPath>> pathAndChain = @@ -242,7 +242,7 @@ isPathPredicate(existingFloorPath), ImmutableList.copyOf( Iterables.concat(symlinkChain, ImmutableList.of(symlinkTargetRootedPath)))); - uniquenessKey = FileSymlinkInfiniteExpansionUniquenessValue.key(pathAndChain.getSecond()); + uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond()); fse = new FileSymlinkInfiniteExpansionException( pathAndChain.getFirst(), pathAndChain.getSecond()); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessFunction.java index c276712..eb01407 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessFunction.java
@@ -13,16 +13,23 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; -import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyKey; -/** A {@link SkyFunction} that has the side effect of reporting a file symlink cycle. */ + +/** + * A {@link SkyFunction} that has the side effect of reporting a file symlink cycle. This is + * achieved by forcing the same key for two logically equivalent cycles + * (e.g. ['a' -> 'b' -> 'c' -> 'a'] and ['b' -> 'c' -> 'a' -> 'b']), and letting Skyframe do its + * magic. + */ public class FileSymlinkCycleUniquenessFunction extends AbstractChainUniquenessFunction<RootedPath> { - @Override - protected SkyValue getDummyValue() { - return FileSymlinkCycleUniquenessValue.INSTANCE; + + static SkyKey key(ImmutableList<RootedPath> cycle) { + return ChainUniquenessUtils.key(SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, cycle); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessValue.java deleted file mode 100644 index 8ae52f4..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkCycleUniquenessValue.java +++ /dev/null
@@ -1,34 +0,0 @@ -// Copyright 2014 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.skyframe; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.vfs.RootedPath; -import com.google.devtools.build.skyframe.SkyKey; - -/** - * A value for ensuring that a file symlink cycle is reported exactly once. This is achieved by - * forcing the same value key for two logically equivalent cycles (e.g. ['a' -> 'b' -> 'c' -> 'a'] - * and ['b' -> 'c' -> 'a' -> 'b']), and letting Skyframe do its magic. - */ -class FileSymlinkCycleUniquenessValue extends AbstractChainUniquenessValue { - static final FileSymlinkCycleUniquenessValue INSTANCE = new FileSymlinkCycleUniquenessValue(); - - private FileSymlinkCycleUniquenessValue() { - } - - static SkyKey key(ImmutableList<RootedPath> cycle) { - return AbstractChainUniquenessValue.key(SkyFunctions.FILE_SYMLINK_CYCLE_UNIQUENESS, cycle); - } -}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessFunction.java index 90d9763..bb0680d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessFunction.java
@@ -13,16 +13,23 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyFunction; -import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyKey; -/** A {@link SkyFunction} that has the side effect of reporting a file symlink expansion error. */ +/** + * A {@link SkyFunction} that has the side effect of reporting a file symlink expansion error + * exactly once. This is achieved by forcing the same value key for two logically equivalent + * expansion errors (e.g. ['a' -> 'b' -> 'c' -> 'a/nope'] and ['b' -> 'c' -> 'a' -> 'a/nope']), + * and letting Skyframe do its magic. + */ public class FileSymlinkInfiniteExpansionUniquenessFunction extends AbstractChainUniquenessFunction<RootedPath> { - @Override - protected SkyValue getDummyValue() { - return FileSymlinkInfiniteExpansionUniquenessValue.INSTANCE; + + static SkyKey key(ImmutableList<RootedPath> cycle) { + return ChainUniquenessUtils.key( + SkyFunctions.FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS, cycle); } @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessValue.java deleted file mode 100644 index 36412d36..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileSymlinkInfiniteExpansionUniquenessValue.java +++ /dev/null
@@ -1,39 +0,0 @@ -// Copyright 2015 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.skyframe; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.vfs.RootedPath; -import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; - -/** - * A value for ensuring that a file symlink expansion error is reported exactly once. This is - * achieved by forcing the same value key for two logically equivalent expansion errors (e.g. - * ['a' -> 'b' -> 'c' -> 'a/nope'] and ['b' -> 'c' -> 'a' -> 'a/nope']), and letting Skyframe do - * its magic. - */ -class FileSymlinkInfiniteExpansionUniquenessValue implements SkyValue { - static final FileSymlinkInfiniteExpansionUniquenessValue INSTANCE = - new FileSymlinkInfiniteExpansionUniquenessValue(); - - private FileSymlinkInfiniteExpansionUniquenessValue() { - } - - static SkyKey key(ImmutableList<RootedPath> cycle) { - return AbstractChainUniquenessValue.key( - SkyFunctions.FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS, cycle); - } -} -
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java index b510f15..c0c72f0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -138,7 +138,7 @@ ImmutableList<PackageIdentifier> cycle = CycleUtils.splitIntoPathAndChain(Predicates.equalTo(pkgId), visited) .second; - if (env.getValue(SkylarkImportUniqueCycleValue.key(cycle)) == null) { + if (env.getValue(SkylarkImportUniqueCycleFunction.key(cycle)) == null) { return null; } throw new SkylarkImportLookupFunctionException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java index a16ad9a..bffbf00 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleFunction.java
@@ -13,15 +13,19 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.skyframe.SkyValue; +import com.google.devtools.build.skyframe.SkyKey; /** * Emits an error message exactly once when a Skylark import cycle is found when running inlined * {@link SkylarkImportLookupFunction}s. */ class SkylarkImportUniqueCycleFunction extends AbstractChainUniquenessFunction<PackageIdentifier> { - private static final SkyValue INSTANCE = new SkyValue() {}; + + static SkyKey key(ImmutableList<PackageIdentifier> cycle) { + return ChainUniquenessUtils.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle); + } @Override protected String getConciseDescription() { @@ -39,11 +43,6 @@ } @Override - protected SkyValue getDummyValue() { - return INSTANCE; - } - - @Override protected String elementToString(PackageIdentifier pkgId) { return pkgId.getPathFragment().getPathString(); }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java deleted file mode 100644 index 0b9df89..0000000 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportUniqueCycleValue.java +++ /dev/null
@@ -1,29 +0,0 @@ -// Copyright 2015 The Bazel Authors. All rights reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -package com.google.devtools.build.lib.skyframe; - -import com.google.common.collect.ImmutableList; -import com.google.devtools.build.lib.cmdline.PackageIdentifier; -import com.google.devtools.build.skyframe.SkyKey; -import com.google.devtools.build.skyframe.SkyValue; - -/** - * If {@link SkylarkImportLookupFunction} is inlined, this is used to emit cycle errors exactly - * once. - */ -public class SkylarkImportUniqueCycleValue implements SkyValue { - static SkyKey key(ImmutableList<PackageIdentifier> cycle) { - return AbstractChainUniquenessValue.key(SkyFunctions.SKYLARK_IMPORT_CYCLE, cycle); - } -}
diff --git a/src/main/java/com/google/devtools/build/skyframe/EmptySkyValue.java b/src/main/java/com/google/devtools/build/skyframe/EmptySkyValue.java new file mode 100644 index 0000000..723e83e --- /dev/null +++ b/src/main/java/com/google/devtools/build/skyframe/EmptySkyValue.java
@@ -0,0 +1,25 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.skyframe; + +/** + * A SkyValue with no attached data. Preferable to a specialized empty value class to minimize + * bloat. + */ +public final class EmptySkyValue implements SkyValue { + public static final EmptySkyValue INSTANCE = new EmptySkyValue(); + + private EmptySkyValue() {} +} +