authorgravatar for stephen@hexops.comStephen Gutekanst <stephen@hexops.com> 2021-12-14 01:10:43-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2021-12-14 13:56:35-08:00
loge563b166b2b70975899c84beb425c8739d05ed65
treebbf22188ad607631e5e6220e1b4f6a4ef1170521
parent5d1aab72d92d0ab512464af0d3ffc036467ac011

Fix compilation cache updating bug leading to slow builds

While investigating slow build times with [a large project](https://github.com/hexops/mach/issues/124), I found that the compiler was reading from disk nearly every C source file in my project when rebuilding despite no changes having been made. This accounted for several seconds of time (approx. 20-30% of running `zig build` without any changes to the sources.) The cause of this was that comparisons of file mtimes would _always_ fail (the mtime of the file on disk was always newer than that stored in the cache manifest), and so the cache logic would always fall back to byte-for-byte file content comparisons with what is on disk vs. in the cache-reading every C source file in my project from disk during each rebuild. Because file contents were the same, a cache hit occurred, and _despite the mtime being different the cache manifest would not be updated._ One can reproduce this by building a Zig project so the cache is populated, and then changing mtimes of their C source files to be newer than what is in the cache (without altering file contents.) The fix is rather simple: we should always write the updated cache manifest regardless of whether or not a cache hit occurred (a cache hit doesn't indicate if a manifest is dirty) Luckily, `writeManifest` already contains logic to determine if a manifest is dirty and becomes no-op if no change to the manifest file is necessary-so we merely need to ensure it is invoked. Signed-off-by: Stephen Gutekanst <stephen@hexops.com>

1 files changed, 16 insertions(+), 8 deletions(-)

src/Compilation.zig+16-8
......@@ -2967,13 +2967,17 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
29672967
29682968 try out_zig_file.writeAll(formatted);
29692969
2970 man.writeManifest() catch |err| {
2971 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
2972 };
2973
29742970 break :digest digest;
29752971 } else man.final();
29762972
2973 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
2974 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
2975 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
2976 // it to prevent doing a full file content comparison the next time around.
2977 man.writeManifest() catch |err| {
2978 log.warn("failed to write cache manifest for C import: {s}", .{@errorName(err)});
2979 };
2980
29772981 const out_zig_path = try comp.local_cache_directory.join(comp.gpa, &[_][]const u8{
29782982 "o", &digest, cimport_zig_basename,
29792983 });
......@@ -3288,13 +3292,17 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
32883292 defer o_dir.close();
32893293 const tmp_basename = std.fs.path.basename(out_obj_path);
32903294 try std.fs.rename(zig_cache_tmp_dir, tmp_basename, o_dir, o_basename);
3291
3292 man.writeManifest() catch |err| {
3293 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
3294 };
32953295 break :blk digest;
32963296 };
32973297
3298 // Write the updated manifest. This is a no-op if the manifest is not dirty. Note that it is
3299 // possible we had a hit and the manifest is dirty, for example if the file mtime changed but
3300 // the contents were the same, we hit the cache but the manifest is dirty and we need to update
3301 // it to prevent doing a full file content comparison the next time around.
3302 man.writeManifest() catch |err| {
3303 log.warn("failed to write cache manifest when compiling '{s}': {s}", .{ c_object.src.src_path, @errorName(err) });
3304 };
3305
32983306 const o_basename = try std.fmt.allocPrint(arena, "{s}{s}", .{ o_basename_noext, o_ext });
32993307
33003308 c_object.status = .{