Skip to content

Commit

Permalink
fix(cache): handle missing cache hits when chaining two run steps
Browse files Browse the repository at this point in the history
fixes ziglang#19817

This improves the efficiency of the cache when chaining muliple commands
like

    const step1 = b.addRunArtifact(tool_fast);
    step1.addFileArg(b.path("src/input.c"));
    const output1 = step1.addOutputFileArg("output1.h");

    const step2 = b.addRunArtifact(tool_slow);
    step2.addFileArg(output1);
    const chained_output = step2.addOutputFileArg("output2.h");

assume that step2 takes much long time than step1
if we make a change to "src/input.c" which produces an identical
"output1.h" as a previous input, one would expect step2 not to
rerun as the cached output2.h only depends on the content of output1.h

However, this does not work yet as the hash of src/input.c leaks into
the file name of the cached output1.h, which the second run step
interprets as a different cache key. Erasing the "zig-build/o/{HASH}"
part of the file name in the hash key fixes this.
  • Loading branch information
bfredl committed May 15, 2024
1 parent 6a65561 commit 4c55582
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 39 deletions.
4 changes: 4 additions & 0 deletions lib/std/Build.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,9 @@ pub const LazyPath = union(enum) {

/// Applied after `up`.
sub_path: []const u8 = "",

/// If true, the file is hashed only on the suffix, not the full absolute path
content_hashed: bool = false,
},

/// An absolute path or a path relative to the current working directory of
Expand Down Expand Up @@ -2339,6 +2342,7 @@ pub const LazyPath = union(enum) {
.file = gen.file,
.up = gen.up,
.sub_path = b.dupePath(gen.sub_path),
.content_hashed = gen.content_hashed,
} },
.dependency => |dep| .{ .dependency = dep },
};
Expand Down
49 changes: 30 additions & 19 deletions lib/std/Build/Cache.zig
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ pub const Manifest = struct {
/// ```
/// var file_contents = cache_hash.files.keys()[file_index].contents.?;
/// ```
pub fn addFile(self: *Manifest, file_path: []const u8, max_file_size: ?usize) !usize {
pub fn addFile(self: *Manifest, file_path: []const u8, max_file_size: ?usize, content_hashed: bool) !usize {
assert(self.manifest_file == null);

const gpa = self.cache.gpa;
Expand All @@ -378,22 +378,33 @@ pub const Manifest = struct {
.bin_digest = undefined,
};

self.hash.add(prefixed_path.prefix);
self.hash.addBytes(prefixed_path.sub_path);
const content_prefix = "zig-cache" ++ std.fs.path.sep_str ++ "o";
// if a file is hashed based on content, we want to skip the zig-build/o/{HASH}/ part
// as the HASH will represent the content of a previous step
// but keep the suffix as it might be relevant to a custom command
if (content_hashed and prefixed_path.sub_path.len > content_prefix.len and
mem.eql(u8, prefixed_path.sub_path[0..content_prefix.len], content_prefix))
{
self.hash.add(@as(u8, '?'));
self.hash.addBytes(prefixed_path.sub_path[content_prefix.len + hex_digest_len + 1 ..]);
} else {
self.hash.add(prefixed_path.prefix);
self.hash.addBytes(prefixed_path.sub_path);
}

return gop.index;
}

pub fn addOptionalFile(self: *Manifest, optional_file_path: ?[]const u8) !void {
self.hash.add(optional_file_path != null);
const file_path = optional_file_path orelse return;
_ = try self.addFile(file_path, null);
_ = try self.addFile(file_path, null, false);
}

pub fn addListOfFiles(self: *Manifest, list_of_files: []const []const u8) !void {
self.hash.add(list_of_files.len);
for (list_of_files) |file_path| {
_ = try self.addFile(file_path, null);
_ = try self.addFile(file_path, null, false);
}
}

Expand Down Expand Up @@ -506,16 +517,14 @@ pub const Manifest = struct {
if (prefix >= self.cache.prefixes_len) return error.InvalidFormat;

if (file_path.len == 0) return error.InvalidFormat;
const prefixed_path: PrefixedPath = .{
.prefix = prefix,
.sub_path = file_path, // expires with file_contents
};

const cache_hash_file = f: {
const prefixed_path: PrefixedPath = .{
.prefix = prefix,
.sub_path = file_path, // expires with file_contents
};
if (idx < input_file_count) {
const file = &self.files.keys()[idx];
if (!file.prefixed_path.eql(prefixed_path))
return error.InvalidFormat;

file.stat = .{
.size = stat_size,
Expand Down Expand Up @@ -561,11 +570,13 @@ pub const Manifest = struct {
self.failed_file_index = idx;
return err;
};

const name_match = pp.eql(prefixed_path);
const size_match = actual_stat.size == cache_hash_file.stat.size;
const mtime_match = actual_stat.mtime == cache_hash_file.stat.mtime;
const inode_match = actual_stat.inode == cache_hash_file.stat.inode;

if (!size_match or !mtime_match or !inode_match) {
if (!name_match or !size_match or !mtime_match or !inode_match) {
self.manifest_dirty = true;

cache_hash_file.stat = .{
Expand Down Expand Up @@ -1078,7 +1089,7 @@ test "cache file and then recall it" {
ch.hash.add(true);
ch.hash.add(@as(u16, 1234));
ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file, null);
_ = try ch.addFile(temp_file, null, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1093,7 +1104,7 @@ test "cache file and then recall it" {
ch.hash.add(true);
ch.hash.add(@as(u16, 1234));
ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file, null);
_ = try ch.addFile(temp_file, null, false);

// Cache hit! We just "built" the same file
try testing.expect(try ch.hit());
Expand Down Expand Up @@ -1144,7 +1155,7 @@ test "check that changing a file makes cache fail" {
defer ch.deinit();

ch.hash.addBytes("1234");
const temp_file_idx = try ch.addFile(temp_file, 100);
const temp_file_idx = try ch.addFile(temp_file, 100, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1163,7 +1174,7 @@ test "check that changing a file makes cache fail" {
defer ch.deinit();

ch.hash.addBytes("1234");
const temp_file_idx = try ch.addFile(temp_file, 100);
const temp_file_idx = try ch.addFile(temp_file, 100, false);

// A file that we depend on has been updated, so the cache should not contain an entry for it
try testing.expectEqual(false, try ch.hit());
Expand Down Expand Up @@ -1267,7 +1278,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

// There should be nothing in the cache
try testing.expectEqual(false, try ch.hit());
Expand All @@ -1282,7 +1293,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

try testing.expect(try ch.hit());
digest2 = ch.final();
Expand All @@ -1305,7 +1316,7 @@ test "Manifest with files added after initial hash work" {
defer ch.deinit();

ch.hash.addBytes("1234");
_ = try ch.addFile(temp_file1, null);
_ = try ch.addFile(temp_file1, null, false);

// A file that we depend on has been updated, so the cache should not contain an entry for it
try testing.expectEqual(false, try ch.hit());
Expand Down
2 changes: 1 addition & 1 deletion lib/std/Build/Step/ObjCopy.zig
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
man.hash.add(@as(u32, 0xe18b7baf));

const full_src_path = objcopy.input_file.getPath2(b, step);
_ = try man.addFile(full_src_path, null);
_ = try man.addFile(full_src_path, null, false);
man.hash.addOptionalListOfBytes(objcopy.only_sections);
man.hash.addOptional(objcopy.pad_to);
man.hash.addOptional(objcopy.format);
Expand Down
16 changes: 10 additions & 6 deletions lib/std/Build/Step/Run.zig
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ pub fn addPrefixedOutputFileArg(
run.setName(b.fmt("{s} ({s})", .{ run.step.name, basename }));
}

return .{ .generated = .{ .file = &output.generated_file } };
return .{ .generated = .{ .file = &output.generated_file, .content_hashed = true } };
}

/// Appends an input file to the command line arguments.
Expand Down Expand Up @@ -596,7 +596,11 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
const file_path = file.lazy_path.getPath2(b, step);
try argv_list.append(b.fmt("{s}{s}", .{ file.prefix, file_path }));
man.hash.addBytes(file.prefix);
_ = try man.addFile(file_path, null);
const content_hashed = switch (file.lazy_path) {
.generated => |gf| gf.content_hashed,
else => false,
};
_ = try man.addFile(file_path, null, content_hashed);
},
.directory_source => |file| {
const file_path = file.lazy_path.getPath2(b, step);
Expand All @@ -613,7 +617,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {

try argv_list.append(file_path);

_ = try man.addFile(file_path, null);
_ = try man.addFile(file_path, null, false);
},
.output_file, .output_directory => |output| {
man.hash.addBytes(output.prefix);
Expand All @@ -637,7 +641,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
},
.lazy_path => |lazy_path| {
const file_path = lazy_path.getPath2(b, step);
_ = try man.addFile(file_path, null);
_ = try man.addFile(file_path, null, true);
},
.none => {},
}
Expand All @@ -653,10 +657,10 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
hashStdIo(&man.hash, run.stdio);

for (run.extra_file_dependencies) |file_path| {
_ = try man.addFile(b.pathFromRoot(file_path), null);
_ = try man.addFile(b.pathFromRoot(file_path), null, false);
}
for (run.file_inputs.items) |lazy_path| {
_ = try man.addFile(lazy_path.getPath2(b, step), null);
_ = try man.addFile(lazy_path.getPath2(b, step), null, false);
}

if (try step.cacheHit(&man) and !has_side_effects) {
Expand Down
2 changes: 1 addition & 1 deletion lib/std/Build/Step/WriteFile.zig
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ fn make(step: *Step, prog_node: *std.Progress.Node) !void {
man.hash.addBytes(bytes);
},
.copy => |file_source| {
_ = try man.addFile(file_source.getPath2(b, step), null);
_ = try man.addFile(file_source.getPath2(b, step), null, true);
},
}
}
Expand Down
14 changes: 7 additions & 7 deletions src/Compilation.zig
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ pub const cache_helpers = struct {
}

pub fn hashCSource(self: *Cache.Manifest, c_source: CSourceFile) !void {
_ = try self.addFile(c_source.src_path, null);
_ = try self.addFile(c_source.src_path, null, false);
// Hash the extra flags, with special care to call addFile for file parameters.
// TODO this logic can likely be improved by utilizing clang_options_data.zig.
const file_args = [_][]const u8{"-include"};
Expand Down Expand Up @@ -2403,13 +2403,13 @@ fn addNonIncrementalStuffToCacheManifest(
}

for (comp.objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
man.hash.add(obj.loption);
}

for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.src.src_path, null);
_ = try man.addFile(key.src.src_path, null, false);
man.hash.addOptional(key.src.ext);
man.hash.addListOfBytes(key.src.extra_flags);
}
Expand All @@ -2418,11 +2418,11 @@ fn addNonIncrementalStuffToCacheManifest(
for (comp.win32_resource_table.keys()) |key| {
switch (key.src) {
.rc => |rc_src| {
_ = try man.addFile(rc_src.src_path, null);
_ = try man.addFile(rc_src.src_path, null, false);
man.hash.addListOfBytes(rc_src.extra_flags);
},
.manifest => |manifest_path| {
_ = try man.addFile(manifest_path, null);
_ = try man.addFile(manifest_path, null, false);
},
}
}
Expand Down Expand Up @@ -4770,7 +4770,7 @@ fn updateWin32Resource(comp: *Compilation, win32_resource: *Win32Resource, win32
// the XML data as a RT_MANIFEST resource. This means we can skip preprocessing,
// include paths, CLI options, etc.
if (win32_resource.src == .manifest) {
_ = try man.addFile(src_path, null);
_ = try man.addFile(src_path, null, false);

const rc_basename = try std.fmt.allocPrint(arena, "{s}.rc", .{src_basename});
const res_basename = try std.fmt.allocPrint(arena, "{s}.res", .{src_basename});
Expand Down Expand Up @@ -4853,7 +4853,7 @@ fn updateWin32Resource(comp: *Compilation, win32_resource: *Win32Resource, win32
// We now know that we're compiling an .rc file
const rc_src = win32_resource.src.rc;

_ = try man.addFile(rc_src.src_path, null);
_ = try man.addFile(rc_src.src_path, null, false);
man.hash.addListOfBytes(rc_src.extra_flags);

const rc_basename_noext = src_basename[0 .. src_basename.len - std.fs.path.extension(src_basename).len];
Expand Down
2 changes: 1 addition & 1 deletion src/glibc.zig
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ pub fn buildSharedObjects(comp: *Compilation, prog_node: *std.Progress.Node) !vo
man.hash.add(target_version);

const full_abilists_path = try comp.zig_lib_directory.join(arena, &.{abilists_path});
const abilists_index = try man.addFile(full_abilists_path, abilists_max_size);
const abilists_index = try man.addFile(full_abilists_path, abilists_max_size, false);

if (try man.hit()) {
const digest = man.final();
Expand Down
8 changes: 4 additions & 4 deletions src/link.zig
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub fn hashAddSystemLibs(
for (hm.values()) |value| {
man.hash.add(value.needed);
man.hash.add(value.weak);
if (value.path) |p| _ = try man.addFile(p, null);
if (value.path) |p| _ = try man.addFile(p, null, false);
}
}

Expand Down Expand Up @@ -736,16 +736,16 @@ pub const File = struct {
base.releaseLock();

for (objects) |obj| {
_ = try man.addFile(obj.path, null);
_ = try man.addFile(obj.path, null, false);
man.hash.add(obj.must_link);
man.hash.add(obj.loption);
}
for (comp.c_object_table.keys()) |key| {
_ = try man.addFile(key.status.success.object_path, null);
_ = try man.addFile(key.status.success.object_path, null, false);
}
if (!build_options.only_core_functionality) {
for (comp.win32_resource_table.keys()) |key| {
_ = try man.addFile(key.status.success.res_path, null);
_ = try man.addFile(key.status.success.res_path, null, false);
}
}
try man.addOptionalFile(zcu_obj_path);
Expand Down

0 comments on commit 4c55582

Please sign in to comment.