Allow FilesystemValueChecker to operate on a WalkableGraph and add TODOs for replacing MemoizingEvaluator#getValues et al with WalkableGraph usage. I initially attempted to do this but punted once I realized how much work it would be.
Also make DirectoryListingStateValue and FileStateValue public for use in outside callers of FilesystemValueChecker.
--
MOS_MIGRATED_REVID=107447425
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index 4c7efc3..0e8a5cf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -15,8 +15,6 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
-import com.google.common.base.Supplier;
-import com.google.common.base.Suppliers;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Range;
@@ -35,15 +33,16 @@
import com.google.devtools.build.lib.vfs.BatchStat;
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
import com.google.devtools.build.skyframe.Differencer;
-import com.google.devtools.build.skyframe.MemoizingEvaluator;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import com.google.devtools.build.skyframe.WalkableGraph;
import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -61,7 +60,7 @@
* A helper class to find dirty values by accessing the filesystem directly (contrast with
* {@link DiffAwareness}).
*/
-class FilesystemValueChecker {
+public class FilesystemValueChecker {
private static final int DIRTINESS_CHECK_THREADS = 200;
private static final Logger LOG = Logger.getLogger(FilesystemValueChecker.class.getName());
@@ -72,70 +71,102 @@
private final TimestampGranularityMonitor tsgm;
@Nullable
private final Range<Long> lastExecutionTimeRange;
- private final Supplier<Map<SkyKey, SkyValue>> valuesSupplier;
private AtomicInteger modifiedOutputFilesCounter = new AtomicInteger(0);
private AtomicInteger modifiedOutputFilesIntraBuildCounter = new AtomicInteger(0);
- FilesystemValueChecker(Supplier<Map<SkyKey, SkyValue>> valuesSupplier,
- @Nullable TimestampGranularityMonitor tsgm, @Nullable Range<Long> lastExecutionTimeRange) {
- this.valuesSupplier = valuesSupplier;
+ public FilesystemValueChecker(@Nullable TimestampGranularityMonitor tsgm,
+ @Nullable Range<Long> lastExecutionTimeRange) {
this.tsgm = tsgm;
this.lastExecutionTimeRange = lastExecutionTimeRange;
}
- FilesystemValueChecker(final MemoizingEvaluator evaluator,
- @Nullable TimestampGranularityMonitor tsgm, @Nullable Range<Long> lastExecutionTimeRange) {
- this.tsgm = tsgm;
- this.lastExecutionTimeRange = lastExecutionTimeRange;
-
- // Construct the full map view of the entire graph at most once ("memoized"), lazily. If
- // getDirtyFilesystemValues(Iterable<SkyKey>) is called on an empty Iterable, we avoid having
- // to create the Map of value keys to values. This is useful in the case where the graph
- // getValues() method could be slow.
- this.valuesSupplier = Suppliers.memoize(new Supplier<Map<SkyKey, SkyValue>>() {
- @Override
- public Map<SkyKey, SkyValue> get() {
- return evaluator.getValues();
- }
- });
- }
-
/**
- * Returns a {@link Differencer.DiffWithDelta} containing keys from the backing graph (of the
- * {@link MemoizingEvaluator} given at construction time) that are dirty according to the
- * passed-in {@code dirtinessChecker}.
+ * Returns a {@link Differencer.DiffWithDelta} containing keys from the give map that are dirty
+ * according to the passed-in {@code dirtinessChecker}.
*/
- Differencer.DiffWithDelta getDirtyKeys(SkyValueDirtinessChecker dirtinessChecker)
- throws InterruptedException {
- return getDirtyValues(valuesSupplier.get().keySet(), dirtinessChecker,
- /*checkMissingValues=*/false);
+ // TODO(bazel-team): Refactor these methods so that FilesystemValueChecker only operates on a
+ // WalkableGraph.
+ Differencer.DiffWithDelta getDirtyKeys(Map<SkyKey, SkyValue> valuesMap,
+ SkyValueDirtinessChecker dirtinessChecker) throws InterruptedException {
+ return getDirtyValues(new MapBackedValueFetcher(valuesMap), valuesMap.keySet(),
+ dirtinessChecker, /*checkMissingValues=*/false);
}
/**
* Returns a {@link Differencer.DiffWithDelta} containing keys that are dirty according to the
* passed-in {@code dirtinessChecker}.
*/
- Differencer.DiffWithDelta getNewAndOldValues(Iterable<SkyKey> keys,
- SkyValueDirtinessChecker dirtinessChecker) throws InterruptedException {
- return getDirtyValues(keys, dirtinessChecker, /*checkMissingValues=*/true);
+ public Differencer.DiffWithDelta getNewAndOldValues(Map<SkyKey, SkyValue> valuesMap,
+ Iterable<SkyKey> keys, SkyValueDirtinessChecker dirtinessChecker)
+ throws InterruptedException {
+ return getDirtyValues(new MapBackedValueFetcher(valuesMap), keys,
+ dirtinessChecker, /*checkMissingValues=*/true);
+ }
+
+ /**
+ * Returns a {@link Differencer.DiffWithDelta} containing keys that are dirty according to the
+ * passed-in {@code dirtinessChecker}.
+ */
+ public Differencer.DiffWithDelta getNewAndOldValues(WalkableGraph walkableGraph,
+ Iterable<SkyKey> keys, SkyValueDirtinessChecker dirtinessChecker)
+ throws InterruptedException {
+ return getDirtyValues(new WalkableGraphBackedValueFetcher(walkableGraph), keys,
+ dirtinessChecker, /*checkMissingValues=*/true);
+ }
+
+ private static interface ValueFetcher {
+ @Nullable
+ SkyValue get(SkyKey key);
+ }
+
+ private static class WalkableGraphBackedValueFetcher implements ValueFetcher {
+ private final WalkableGraph walkableGraph;
+
+ private WalkableGraphBackedValueFetcher(WalkableGraph walkableGraph) {
+ this.walkableGraph = walkableGraph;
+ }
+
+ @Override
+ @Nullable
+ public SkyValue get(SkyKey key) {
+ return walkableGraph.exists(key) ? walkableGraph.getValue(key) : null;
+ }
+ }
+
+ private static class MapBackedValueFetcher implements ValueFetcher {
+ private final Map<SkyKey, SkyValue> valuesMap;
+
+ private MapBackedValueFetcher(Map<SkyKey, SkyValue> valuesMap) {
+ this.valuesMap = valuesMap;
+ }
+
+ @Override
+ @Nullable
+ public SkyValue get(SkyKey key) {
+ return valuesMap.get(key);
+ }
}
/**
* Return a collection of action values which have output files that are not in-sync with
* the on-disk file value (were modified externally).
*/
- Collection<SkyKey> getDirtyActionValues(@Nullable final BatchStat batchStatter)
- throws InterruptedException {
+ Collection<SkyKey> getDirtyActionValues(Map<SkyKey, SkyValue> valuesMap,
+ @Nullable final BatchStat batchStatter) throws InterruptedException {
// CPU-bound (usually) stat() calls, plus a fudge factor.
LOG.info("Accumulating dirty actions");
final int numOutputJobs = Runtime.getRuntime().availableProcessors() * 4;
- final Set<SkyKey> actionSkyKeys =
- Sets.filter(valuesSupplier.get().keySet(), ACTION_FILTER);
+ final Set<SkyKey> actionSkyKeys = new HashSet<>();
+ for (SkyKey key : valuesMap.keySet()) {
+ if (ACTION_FILTER.apply(key)) {
+ actionSkyKeys.add(key);
+ }
+ }
final Sharder<Pair<SkyKey, ActionExecutionValue>> outputShards =
new Sharder<>(numOutputJobs, actionSkyKeys.size());
for (SkyKey key : actionSkyKeys) {
- outputShards.add(Pair.of(key, (ActionExecutionValue) valuesSupplier.get().get(key)));
+ outputShards.add(Pair.of(key, (ActionExecutionValue) valuesMap.get(key)));
}
LOG.info("Sharded action values for batching");
@@ -285,8 +316,8 @@
return isDirty;
}
- private BatchDirtyResult getDirtyValues(
- Iterable<SkyKey> values, final SkyValueDirtinessChecker checker,
+ private BatchDirtyResult getDirtyValues(ValueFetcher fetcher,
+ Iterable<SkyKey> keys, final SkyValueDirtinessChecker checker,
final boolean checkMissingValues) throws InterruptedException {
ExecutorService executor =
Executors.newFixedThreadPool(
@@ -310,23 +341,24 @@
}
};
try (AutoProfiler prof = AutoProfiler.create(elapsedTimeReceiver)) {
- for (final SkyKey key : values) {
+ for (final SkyKey key : keys) {
numKeysScanned.incrementAndGet();
if (!checker.applies(key)) {
continue;
}
- final SkyValue value = valuesSupplier.get().get(key);
+ final SkyValue value = fetcher.get(key);
+ if (!checkMissingValues && value == null) {
+ continue;
+ }
executor.execute(
wrapper.wrap(
new Runnable() {
@Override
public void run() {
- if (value != null || checkMissingValues) {
- numKeysChecked.incrementAndGet();
- DirtyResult result = checker.check(key, value, tsgm);
- if (result.isDirty()) {
- batchResult.add(key, value, result.getNewValue());
- }
+ numKeysChecked.incrementAndGet();
+ DirtyResult result = checker.check(key, value, tsgm);
+ if (result.isDirty()) {
+ batchResult.add(key, value, result.getNewValue());
}
}
}));
@@ -342,7 +374,7 @@
}
/**
- * Result of a batch call to {@link SkyValueDirtinessChecker#maybeCheck}. Partitions the dirty
+ * Result of a batch call to {@link SkyValueDirtinessChecker#check}. Partitions the dirty
* values based on whether we have a new value available for them or not.
*/
private static class BatchDirtyResult implements Differencer.DiffWithDelta {