Skip to content

Commit 50d26d1

Browse files
janakdrcopybara-github
janakdr
authored and
copybara-github
committed
Modify contract of SyscallCache#getType to allow the cache to do no work if it prefers. This prevents SyscallCache#NO_CACHE from being less efficient in codepaths that call #getType than just not using a cache at all.
I need this so that when I change remaining callsites of Path#getFastDigest to require a SyscallCache, any callers of those callsites can pass in SyscallCache#NO_CACHE without a loss of performance. PiperOrigin-RevId: 430606848
1 parent ba348cc commit 50d26d1

File tree

18 files changed

+197
-80
lines changed

18 files changed

+197
-80
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/FileStateValue.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,15 @@ public static FileStateValue create(
7272
RootedPath rootedPath, SyscallCache syscallCache, @Nullable TimestampGranularityMonitor tsgm)
7373
throws IOException {
7474
Path path = rootedPath.asPath();
75-
Dirent.Type type = syscallCache.getType(path, Symlinks.NOFOLLOW);
75+
SyscallCache.DirentTypeWithSkip typeWithSkip = syscallCache.getType(path, Symlinks.NOFOLLOW);
76+
FileStatus stat = null;
77+
Dirent.Type type = null;
78+
if (typeWithSkip == SyscallCache.DirentTypeWithSkip.FILESYSTEM_OP_SKIPPED) {
79+
stat = syscallCache.statIfFound(path, Symlinks.NOFOLLOW);
80+
type = SyscallCache.statusToDirentType(stat);
81+
} else if (typeWithSkip != null) {
82+
type = typeWithSkip.getType();
83+
}
7684
if (type == null) {
7785
return NONEXISTENT_FILE_STATE_NODE;
7886
}
@@ -83,7 +91,9 @@ public static FileStateValue create(
8391
return new SymlinkFileStateValue(path.readSymbolicLinkUnchecked());
8492
case FILE:
8593
case UNKNOWN:
86-
FileStatus stat = syscallCache.statIfFound(path, Symlinks.NOFOLLOW);
94+
if (stat == null) {
95+
stat = syscallCache.statIfFound(path, Symlinks.NOFOLLOW);
96+
}
8797
if (stat == null) {
8898
throw new InconsistentFilesystemException(
8999
"File " + rootedPath + " found in directory, but stat failed");

‎src/main/java/com/google/devtools/build/lib/pkgcache/PathPackageLocator.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,13 @@ private Path getFilePath(PathFragment suffix, SyscallCache cache) {
237237
for (Root pathEntry : pathEntries) {
238238
Path buildFile = pathEntry.getRelative(suffix);
239239
try {
240-
Dirent.Type type = cache.getType(buildFile, Symlinks.FOLLOW);
240+
SyscallCache.DirentTypeWithSkip typeWithSkip = cache.getType(buildFile, Symlinks.FOLLOW);
241+
Dirent.Type type = null;
242+
if (typeWithSkip == SyscallCache.DirentTypeWithSkip.FILESYSTEM_OP_SKIPPED) {
243+
type = SyscallCache.statusToDirentType(cache.statIfFound(buildFile, Symlinks.FOLLOW));
244+
} else if (typeWithSkip != null) {
245+
type = typeWithSkip.getType();
246+
}
241247
if (type == Dirent.Type.FILE || type == Dirent.Type.UNKNOWN) {
242248
return buildFile;
243249
}

‎src/main/java/com/google/devtools/build/lib/skyframe/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1829,6 +1829,7 @@ java_library(
18291829
"//src/main/java/com/google/devtools/build/lib/vfs",
18301830
"//third_party:caffeine",
18311831
"//third_party:guava",
1832+
"//third_party:jsr305",
18321833
],
18331834
)
18341835

‎src/main/java/com/google/devtools/build/lib/skyframe/PerBuildSyscallCache.java

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import java.io.IOException;
2727
import java.util.Collection;
2828
import java.util.function.Supplier;
29+
import javax.annotation.Nullable;
2930

3031
/**
3132
* A per-build cache of filesystem operations.
@@ -135,7 +136,7 @@ public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
135136

136137
@Override
137138
@SuppressWarnings("unchecked")
138-
public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
139+
public DirentTypeWithSkip getType(Path path, Symlinks symlinks) throws IOException {
139140
// Use a cached stat call if we have one. This is done first so that we don't need to iterate
140141
// over a list of directory entries as we do for cached readdir() entries. We don't ever expect
141142
// to get a cache hit if symlinks == Symlinks.NOFOLLOW and so we don't bother to check.
@@ -146,14 +147,14 @@ public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
146147
if (result == NO_STATUS) {
147148
return null;
148149
}
149-
return SyscallCache.statusToDirentType((FileStatus) result);
150+
return ofStat((FileStatus) result);
150151
}
151152
}
152153

153154
// If this is a root directory, we must stat, there is no parent.
154155
Path parent = path.getParentDirectory();
155156
if (parent == null) {
156-
return SyscallCache.statusToDirentType(statIfFound(path, symlinks));
157+
return ofStat(statIfFound(path, symlinks));
157158
}
158159

159160
// Answer based on a cached readdir() call if possible. The cache might already be populated
@@ -178,14 +179,19 @@ public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
178179
}
179180
if (dirent.getType() == Dirent.Type.SYMLINK && symlinks == Symlinks.FOLLOW) {
180181
// See above: We don't want to follow symlinks with readdir(). Do a stat() instead.
181-
return SyscallCache.statusToDirentType(statIfFound(path, Symlinks.FOLLOW));
182+
return ofStat(statIfFound(path, Symlinks.FOLLOW));
182183
}
183-
return dirent.getType();
184+
return DirentTypeWithSkip.of(dirent.getType());
184185
}
185186
return null;
186187
}
187188

188-
return SyscallCache.statusToDirentType(statIfFound(path, symlinks));
189+
return ofStat(statIfFound(path, symlinks));
190+
}
191+
192+
@Nullable
193+
private static DirentTypeWithSkip ofStat(@Nullable FileStatus status) {
194+
return DirentTypeWithSkip.of(SyscallCache.statusToDirentType(status));
189195
}
190196

191197
@Override

‎src/main/java/com/google/devtools/build/lib/vfs/SyscallCache.java

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,11 @@
1414

1515
package com.google.devtools.build.lib.vfs;
1616

17+
import static com.google.common.base.Preconditions.checkState;
18+
1719
import java.io.IOException;
1820
import java.util.Collection;
21+
import javax.annotation.Nullable;
1922

2023
/**
2124
* Centralized point to perform filesystem calls, to promote caching. Ideally all filesystem
@@ -36,8 +39,8 @@ public FileStatus statIfFound(Path path, Symlinks symlinks) throws IOException {
3639
}
3740

3841
@Override
39-
public Dirent.Type getType(Path path, Symlinks symlinks) throws IOException {
40-
return statusToDirentType(statIfFound(path, symlinks));
42+
public DirentTypeWithSkip getType(Path path, Symlinks symlinks) {
43+
return DirentTypeWithSkip.FILESYSTEM_OP_SKIPPED;
4144
}
4245

4346
@Override
@@ -52,9 +55,12 @@ public void clear() {}
5255

5356
/**
5457
* Returns the type of a specific file. This may be answered using stat() or readdir(). Returns
55-
* null if the path does not exist.
58+
* null if the path does not exist. Returns {@link DirentTypeWithSkip#FILESYSTEM_OP_SKIPPED} if
59+
* cache had no data for path and chose not to do filesystem access to determine the type. Callers
60+
* should call {@link #statIfFound} and then {@link #statusToDirentType} if needed in that case.
5661
*/
57-
Dirent.Type getType(Path path, Symlinks symlinks) throws IOException;
62+
@Nullable
63+
DirentTypeWithSkip getType(Path path, Symlinks symlinks) throws IOException;
5864

5965
default byte[] getFastDigest(Path path) throws IOException {
6066
return path.getFastDigest();
@@ -71,6 +77,47 @@ default void noteAnalysisPhaseEnded() {
7177
clear();
7278
}
7379

80+
/**
81+
* A {@link Dirent.Type} with an additional element signifying that the type is unknown because
82+
* this {@link SyscallCache} implementation skipped filesystem access.
83+
*/
84+
enum DirentTypeWithSkip {
85+
FILE(Dirent.Type.FILE),
86+
DIRECTORY(Dirent.Type.DIRECTORY),
87+
SYMLINK(Dirent.Type.SYMLINK),
88+
UNKNOWN(Dirent.Type.UNKNOWN),
89+
FILESYSTEM_OP_SKIPPED(null);
90+
91+
@Nullable private final Dirent.Type type;
92+
93+
DirentTypeWithSkip(@Nullable Dirent.Type type) {
94+
this.type = type;
95+
}
96+
97+
public Dirent.Type getType() {
98+
checkState(this != FILESYSTEM_OP_SKIPPED, "No type if filesystem op skipped");
99+
return type;
100+
}
101+
102+
@Nullable
103+
public static DirentTypeWithSkip of(@Nullable Dirent.Type type) {
104+
if (type == null) {
105+
return null;
106+
}
107+
switch (type) {
108+
case FILE:
109+
return FILE;
110+
case DIRECTORY:
111+
return DIRECTORY;
112+
case SYMLINK:
113+
return SYMLINK;
114+
case UNKNOWN:
115+
return UNKNOWN;
116+
}
117+
throw new IllegalStateException("Got unrecognized type " + type);
118+
}
119+
}
120+
74121
static Dirent.Type statusToDirentType(FileStatus status) {
75122
if (status == null) {
76123
return null;

‎src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,11 @@
8282
import com.google.devtools.build.lib.testutil.TestConstants;
8383
import com.google.devtools.build.lib.testutil.TestConstants.InternalTestExecutionMode;
8484
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
85+
import com.google.devtools.build.lib.testutil.TestUtils;
8586
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
8687
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
8788
import com.google.devtools.build.lib.vfs.PathFragment;
8889
import com.google.devtools.build.lib.vfs.Root;
89-
import com.google.devtools.build.lib.vfs.SyscallCache;
9090
import com.google.devtools.common.options.Options;
9191
import com.google.devtools.common.options.OptionsParser;
9292
import java.util.Arrays;
@@ -163,6 +163,8 @@ public boolean contains(Flag flag) {
163163

164164
protected AnalysisTestUtil.DummyWorkspaceStatusActionFactory workspaceStatusActionFactory;
165165
private PathPackageLocator pkgLocator;
166+
protected final TestUtils.DelegatingSyscallCache delegatingSyscallCache =
167+
new TestUtils.DelegatingSyscallCache();
166168

167169
@Before
168170
public final void createMocks() throws Exception {
@@ -196,7 +198,7 @@ protected SkyframeExecutor createSkyframeExecutor(PackageFactory pkgFactory) {
196198
.setActionKeyContext(actionKeyContext)
197199
.setWorkspaceStatusActionFactory(workspaceStatusActionFactory)
198200
.setExtraSkyFunctions(analysisMock.getSkyFunctions(directories))
199-
.setPerCommandSyscallCache(SyscallCache.NO_CACHE)
201+
.setPerCommandSyscallCache(delegatingSyscallCache)
200202
.build();
201203
}
202204

‎src/test/java/com/google/devtools/build/lib/analysis/util/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ java_library(
134134
"//src/test/java/com/google/devtools/build/lib/testutil",
135135
"//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper",
136136
"//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
137+
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
137138
"//src/test/java/com/google/devtools/build/skyframe:testutil",
138139
"//third_party:auto_value",
139140
"//third_party:error_prone_annotations",

‎src/test/java/com/google/devtools/build/lib/packages/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ java_library(
176176
"//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper",
177177
"//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
178178
"//src/test/java/com/google/devtools/build/lib/testutil:TestPackageFactoryBuilderFactory",
179+
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
179180
"//src/test/java/com/google/devtools/build/lib/vfs/util",
180181
"//third_party:guava",
181182
"//third_party:jsr305",

‎src/test/java/com/google/devtools/build/lib/packages/util/PackageLoadingTestCase.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@
4444
import com.google.devtools.build.lib.testutil.FoundationTestCase;
4545
import com.google.devtools.build.lib.testutil.SkyframeExecutorTestHelper;
4646
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
47+
import com.google.devtools.build.lib.testutil.TestUtils;
4748
import com.google.devtools.build.lib.util.AbruptExitException;
4849
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
4950
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
5051
import com.google.devtools.build.lib.vfs.Root;
51-
import com.google.devtools.build.lib.vfs.SyscallCache;
5252
import com.google.devtools.common.options.Options;
5353
import com.google.devtools.common.options.OptionsParser;
5454
import java.util.List;
@@ -73,6 +73,8 @@ public abstract class PackageLoadingTestCase extends FoundationTestCase {
7373
protected SkyframeExecutor skyframeExecutor;
7474
protected BlazeDirectories directories;
7575
protected PackageValidator validator = null;
76+
protected final TestUtils.DelegatingSyscallCache delegatingSyscallCache =
77+
new TestUtils.DelegatingSyscallCache();
7678

7779
protected final ActionKeyContext actionKeyContext = new ActionKeyContext();
7880

@@ -126,7 +128,7 @@ private SkyframeExecutor createSkyframeExecutor() {
126128
.setFileSystem(fileSystem)
127129
.setDirectories(directories)
128130
.setActionKeyContext(actionKeyContext)
129-
.setPerCommandSyscallCache(SyscallCache.NO_CACHE)
131+
.setPerCommandSyscallCache(delegatingSyscallCache)
130132
.build();
131133
skyframeExecutor.injectExtraPrecomputedValues(
132134
ImmutableList.of(

‎src/test/java/com/google/devtools/build/lib/pkgcache/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ java_test(
6565
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
6666
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
6767
"//src/main/protobuf:failure_details_java_proto",
68+
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
6869
"//third_party:guava",
6970
"//third_party:jsr305",
7071
"//third_party:junit4",

‎src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorIOTest.java

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.google.devtools.build.lib.cmdline.TargetParsingException;
2323
import com.google.devtools.build.lib.events.EventKind;
2424
import com.google.devtools.build.lib.server.FailureDetails;
25+
import com.google.devtools.build.lib.testutil.TestUtils;
2526
import com.google.devtools.build.lib.vfs.DigestHashFunction;
2627
import com.google.devtools.build.lib.vfs.Dirent;
2728
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -130,19 +131,7 @@ public void testBadStat(@TestParameter boolean keepGoing) throws Exception {
130131
public void testBadStatPathAsTarget(@TestParameter boolean keepGoing) throws Exception {
131132
reporter.removeHandler(failFastHandler);
132133
scratch.file("parent/BUILD", "sh_library(name = 'parent')").getParentDirectory();
133-
AtomicBoolean returnNull = new AtomicBoolean(false);
134-
this.transformer =
135-
new Transformer() {
136-
@Nullable
137-
@Override
138-
public FileStatus stat(FileStatus stat, PathFragment path, boolean followSymlinks) {
139-
if (path.endsWith(PathFragment.create("parent/BUILD")) && returnNull.getAndSet(true)) {
140-
return null;
141-
}
142-
return stat;
143-
}
144-
};
145-
134+
delegatingSyscallCache.setDelegate(TestUtils.makeDisappearingFileCache("parent/BUILD"));
146135
TargetParsingException e =
147136
assertThrows(
148137
TargetParsingException.class,

‎src/test/java/com/google/devtools/build/lib/query2/testutil/BUILD

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ java_library(
1717
srcs = glob(["*.java"]),
1818
deps = [
1919
"//src/main/java/com/google/devtools/build/lib/actions",
20-
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
2120
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
2221
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
2322
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
@@ -40,7 +39,6 @@ java_library(
4039
"//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_function",
4140
"//src/main/java/com/google/devtools/build/lib/skyframe:package_value",
4241
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
43-
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
4442
"//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
4543
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
4644
"//src/main/java/com/google/devtools/build/lib/util:filetype",
@@ -59,10 +57,9 @@ java_library(
5957
"//src/test/java/com/google/devtools/build/lib/testutil:SkyframeExecutorTestHelper",
6058
"//src/test/java/com/google/devtools/build/lib/testutil:TestConstants",
6159
"//src/test/java/com/google/devtools/build/lib/testutil:TestPackageFactoryBuilderFactory",
62-
"//src/test/java/com/google/devtools/build/skyframe:testutil",
60+
"//src/test/java/com/google/devtools/build/lib/testutil:TestUtils",
6361
"//third_party:error_prone_annotations",
6462
"//third_party:guava",
65-
"//third_party:jsr305",
6663
"//third_party:junit4",
6764
"//third_party:truth",
6865
],

‎src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryHelper.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import com.google.devtools.build.lib.vfs.FileSystemUtils;
4444
import com.google.devtools.build.lib.vfs.Path;
4545
import com.google.devtools.build.lib.vfs.PathFragment;
46+
import com.google.devtools.build.lib.vfs.SyscallCache;
4647
import com.google.devtools.build.skyframe.SkyKey;
4748
import com.google.devtools.build.skyframe.WalkableGraph;
4849
import java.io.IOException;
@@ -70,14 +71,10 @@ public abstract class PostAnalysisQueryHelper<T> extends AbstractQueryHelper<T>
7071

7172
@Override
7273
public void setUp() throws Exception {
73-
setUp(new AnalysisHelper());
74-
}
75-
76-
public void setUp(AnalysisHelper analysisHelper) throws Exception {
7774
super.setUp();
7875
parserPrefix = PathFragment.EMPTY_FRAGMENT;
79-
this.analysisHelper = analysisHelper;
8076
wholeTestUniverse = false;
77+
this.analysisHelper = new AnalysisHelper();
8178
// Reverse the @Before method list, so that superclass is called before subclass.
8279
for (Method method :
8380
Lists.reverse(getMethodsAnnotatedWith(AnalysisHelper.class, Before.class))) {
@@ -102,6 +99,10 @@ MockToolsConfig getMockToolsConfig() {
10299
return analysisHelper.getMockToolsConfig();
103100
}
104101

102+
void setSyscallCache(SyscallCache syscallCache) {
103+
this.analysisHelper.setSyscallCache(syscallCache);
104+
}
105+
105106
public boolean isWholeTestUniverse() {
106107
return wholeTestUniverse;
107108
}
@@ -302,6 +303,10 @@ protected Reporter getReporter() {
302303
return reporter;
303304
}
304305

306+
private void setSyscallCache(SyscallCache syscallCache) {
307+
this.delegatingSyscallCache.setDelegate(syscallCache);
308+
}
309+
305310
@Override
306311
protected BuildConfigurationValue getTargetConfiguration() throws InterruptedException {
307312
return super.getTargetConfiguration();

0 commit comments

Comments
 (0)