From 06310e3d4eb47fed88b175891cb5865bb050f020 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Fri, 22 Apr 2022 08:19:51 -0700 Subject: [PATCH] Revert "Fix C include files not being in `whole` cache (#11365)" This reverts commit a430630002bf02162ccbf8d3eb10fd73e490cefd. Wait a minute, I'm sorry, I need to revert this. The whole premise of this change is broken because the point of the hash is that it tells whether the same compilation has been done before. This requires items to be added to the hash in the same sequence every time. This means that introducing a lock is fundamentally broken because the order needs to be the same in future runs of the compiler, and not decided by threads racing against each other. The proper solution to this is to, in whole cache mode, append the hash inputs to some data structure, and then after the compilation is complete, do some kind of sorting on the hash inputs so that they will be the same order every time, then apply them in sequence. No lock on the Cache object is needed for this scheme. --- src/Cache.zig | 2 +- src/Compilation.zig | 10 +--------- src/Module.zig | 4 ---- src/stage1.zig | 9 ++++----- 4 files changed, 6 insertions(+), 19 deletions(-) diff --git a/src/Cache.zig b/src/Cache.zig index 2438c5528d0d1ff3ccc0b7373501fa9b43afba2b..0d4b51492de371a6cb3f9217cfe97747949ad60e 100644 --- a/src/Cache.zig +++ b/src/Cache.zig @@ -690,7 +690,7 @@ pub const Manifest = struct { while (true) { switch (it.next() orelse return) { .target, .target_must_resolve => return, - .prereq => |file_path| try self.addFilePost(file_path), + .prereq => |bytes| try self.addFilePost(bytes), else => |err| { try err.printError(error_buf.writer()); log.err("failed parsing {s}: {s}", .{ dep_file_basename, error_buf.items }); diff --git a/src/Compilation.zig b/src/Compilation.zig index d7181c7a0d25d57f00e45459e24d2e913744cf00..bfe52cd59e36e6ae29526c49109e7a94a3b82b1c 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -44,7 +44,6 @@ bin_file: *link.File, c_object_table: std.AutoArrayHashMapUnmanaged(*CObject, void) = .{}, /// This is a pointer to a local variable inside `update()`. whole_cache_manifest: ?*Cache.Manifest = null, -whole_cache_manifest_mutex: std.Thread.Mutex = .{}, link_error_flags: link.File.ErrorFlags = .{}, @@ -1963,8 +1962,8 @@ pub fn update(comp: *Compilation) !void { // We are about to obtain this lock, so here we give other processes a chance first. comp.bin_file.releaseLock(); - man = comp.cache_parent.obtain(); comp.whole_cache_manifest = &man; + man = comp.cache_parent.obtain(); try comp.addNonIncrementalStuffToCacheManifest(&man); const is_hit = man.hit() catch |err| { @@ -3353,8 +3352,6 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult { const dep_basename = std.fs.path.basename(out_dep_path); try man.addDepFilePost(zig_cache_tmp_dir, dep_basename); if (comp.whole_cache_manifest) |whole_cache_manifest| { - comp.whole_cache_manifest_mutex.lock(); - defer comp.whole_cache_manifest_mutex.unlock(); try whole_cache_manifest.addDepFilePost(zig_cache_tmp_dir, dep_basename); } @@ -3696,11 +3693,6 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P const dep_basename = std.fs.path.basename(dep_file_path); // Add the files depended on to the cache system. try man.addDepFilePost(zig_cache_tmp_dir, dep_basename); - if (comp.whole_cache_manifest) |whole_cache_manifest| { - comp.whole_cache_manifest_mutex.lock(); - defer comp.whole_cache_manifest_mutex.unlock(); - try whole_cache_manifest.addDepFilePost(zig_cache_tmp_dir, dep_basename); - } // Just to save disk space, we delete the file because it is never needed again. zig_cache_tmp_dir.deleteFile(dep_basename) catch |err| { log.warn("failed to delete '{s}': {s}", .{ dep_file_path, @errorName(err) }); diff --git a/src/Module.zig b/src/Module.zig index e637a55dfb25cbfc03ac90dc6e1c32cbf7e251b6..1119d73ab04827097c964900676a0afc57ed3508 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -3855,8 +3855,6 @@ pub fn semaFile(mod: *Module, file: *File) SemaError!void { }); errdefer gpa.free(resolved_path); - mod.comp.whole_cache_manifest_mutex.lock(); - defer mod.comp.whole_cache_manifest_mutex.unlock(); try man.addFilePostContents(resolved_path, source.bytes, source.stat); } } else { @@ -4338,8 +4336,6 @@ pub fn embedFile(mod: *Module, cur_file: *File, rel_file_path: []const u8) !*Emb if (mod.comp.whole_cache_manifest) |man| { const copied_resolved_path = try gpa.dupe(u8, resolved_path); errdefer gpa.free(copied_resolved_path); - mod.comp.whole_cache_manifest_mutex.lock(); - defer mod.comp.whole_cache_manifest_mutex.unlock(); try man.addFilePostContents(copied_resolved_path, bytes, stat); } diff --git a/src/stage1.zig b/src/stage1.zig index 2533b242c78d6199d0328478d400c0b76be1b32f..005dc312ba1fd9d2590d72cf7cb461ee5880816a 100644 --- a/src/stage1.zig +++ b/src/stage1.zig @@ -455,11 +455,10 @@ export fn stage2_fetch_file( const comp = @intToPtr(*Compilation, stage1.userdata); const file_path = path_ptr[0..path_len]; const max_file_size = std.math.maxInt(u32); - const contents = if (comp.whole_cache_manifest) |man| blk: { - comp.whole_cache_manifest_mutex.lock(); - defer comp.whole_cache_manifest_mutex.unlock(); - break :blk man.addFilePostFetch(file_path, max_file_size) catch return null; - } else std.fs.cwd().readFileAlloc(comp.gpa, file_path, max_file_size) catch return null; + const contents = if (comp.whole_cache_manifest) |man| + man.addFilePostFetch(file_path, max_file_size) catch return null + else + std.fs.cwd().readFileAlloc(comp.gpa, file_path, max_file_size) catch return null; result_len.* = contents.len; // TODO https://github.com/ziglang/zig/issues/3328#issuecomment-716749475 if (contents.len == 0) return @intToPtr(?[*]const u8, 0x1); -- 2.54.0