authorgravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2022-04-22 08:19:51-07:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2022-04-22 08:19:51-07:00
log06310e3d4eb47fed88b175891cb5865bb050f020
treee19d5d14323ae797495027f33fbb63120fce660b
parenta430630002bf02162ccbf8d3eb10fd73e490cefd

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.

4 files changed, 6 insertions(+), 19 deletions(-)

src/Cache.zig+1-1
...@@ -690,7 +690,7 @@ pub const Manifest = struct {...@@ -690,7 +690,7 @@ pub const Manifest = struct {
690 while (true) {690 while (true) {
691 switch (it.next() orelse return) {691 switch (it.next() orelse return) {
692 .target, .target_must_resolve => return,692 .target, .target_must_resolve => return,
693 .prereq => |file_path| try self.addFilePost(file_path),693 .prereq => |bytes| try self.addFilePost(bytes),
694 else => |err| {694 else => |err| {
695 try err.printError(error_buf.writer());695 try err.printError(error_buf.writer());
696 log.err("failed parsing {s}: {s}", .{ dep_file_basename, error_buf.items });696 log.err("failed parsing {s}: {s}", .{ dep_file_basename, error_buf.items });
src/Compilation.zig+1-9
...@@ -44,7 +44,6 @@ bin_file: *link.File,...@@ -44,7 +44,6 @@ bin_file: *link.File,
44c_object_table: std.AutoArrayHashMapUnmanaged(*CObject, void) = .{},44c_object_table: std.AutoArrayHashMapUnmanaged(*CObject, void) = .{},
45/// This is a pointer to a local variable inside `update()`.45/// This is a pointer to a local variable inside `update()`.
46whole_cache_manifest: ?*Cache.Manifest = null,46whole_cache_manifest: ?*Cache.Manifest = null,
47whole_cache_manifest_mutex: std.Thread.Mutex = .{},
4847
49link_error_flags: link.File.ErrorFlags = .{},48link_error_flags: link.File.ErrorFlags = .{},
5049
...@@ -1963,8 +1962,8 @@ pub fn update(comp: *Compilation) !void {...@@ -1963,8 +1962,8 @@ pub fn update(comp: *Compilation) !void {
1963 // We are about to obtain this lock, so here we give other processes a chance first.1962 // We are about to obtain this lock, so here we give other processes a chance first.
1964 comp.bin_file.releaseLock();1963 comp.bin_file.releaseLock();
19651964
1966 man = comp.cache_parent.obtain();
1967 comp.whole_cache_manifest = &man;1965 comp.whole_cache_manifest = &man;
1966 man = comp.cache_parent.obtain();
1968 try comp.addNonIncrementalStuffToCacheManifest(&man);1967 try comp.addNonIncrementalStuffToCacheManifest(&man);
19691968
1970 const is_hit = man.hit() catch |err| {1969 const is_hit = man.hit() catch |err| {
...@@ -3353,8 +3352,6 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {...@@ -3353,8 +3352,6 @@ pub fn cImport(comp: *Compilation, c_src: []const u8) !CImportResult {
3353 const dep_basename = std.fs.path.basename(out_dep_path);3352 const dep_basename = std.fs.path.basename(out_dep_path);
3354 try man.addDepFilePost(zig_cache_tmp_dir, dep_basename);3353 try man.addDepFilePost(zig_cache_tmp_dir, dep_basename);
3355 if (comp.whole_cache_manifest) |whole_cache_manifest| {3354 if (comp.whole_cache_manifest) |whole_cache_manifest| {
3356 comp.whole_cache_manifest_mutex.lock();
3357 defer comp.whole_cache_manifest_mutex.unlock();
3358 try whole_cache_manifest.addDepFilePost(zig_cache_tmp_dir, dep_basename);3355 try whole_cache_manifest.addDepFilePost(zig_cache_tmp_dir, dep_basename);
3359 }3356 }
33603357
...@@ -3696,11 +3693,6 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P...@@ -3696,11 +3693,6 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P
3696 const dep_basename = std.fs.path.basename(dep_file_path);3693 const dep_basename = std.fs.path.basename(dep_file_path);
3697 // Add the files depended on to the cache system.3694 // Add the files depended on to the cache system.
3698 try man.addDepFilePost(zig_cache_tmp_dir, dep_basename);3695 try man.addDepFilePost(zig_cache_tmp_dir, dep_basename);
3699 if (comp.whole_cache_manifest) |whole_cache_manifest| {
3700 comp.whole_cache_manifest_mutex.lock();
3701 defer comp.whole_cache_manifest_mutex.unlock();
3702 try whole_cache_manifest.addDepFilePost(zig_cache_tmp_dir, dep_basename);
3703 }
3704 // Just to save disk space, we delete the file because it is never needed again.3696 // Just to save disk space, we delete the file because it is never needed again.
3705 zig_cache_tmp_dir.deleteFile(dep_basename) catch |err| {3697 zig_cache_tmp_dir.deleteFile(dep_basename) catch |err| {
3706 log.warn("failed to delete '{s}': {s}", .{ dep_file_path, @errorName(err) });3698 log.warn("failed to delete '{s}': {s}", .{ dep_file_path, @errorName(err) });
src/Module.zig-4
...@@ -3855,8 +3855,6 @@ pub fn semaFile(mod: *Module, file: *File) SemaError!void {...@@ -3855,8 +3855,6 @@ pub fn semaFile(mod: *Module, file: *File) SemaError!void {
3855 });3855 });
3856 errdefer gpa.free(resolved_path);3856 errdefer gpa.free(resolved_path);
38573857
3858 mod.comp.whole_cache_manifest_mutex.lock();
3859 defer mod.comp.whole_cache_manifest_mutex.unlock();
3860 try man.addFilePostContents(resolved_path, source.bytes, source.stat);3858 try man.addFilePostContents(resolved_path, source.bytes, source.stat);
3861 }3859 }
3862 } else {3860 } else {
...@@ -4338,8 +4336,6 @@ pub fn embedFile(mod: *Module, cur_file: *File, rel_file_path: []const u8) !*Emb...@@ -4338,8 +4336,6 @@ pub fn embedFile(mod: *Module, cur_file: *File, rel_file_path: []const u8) !*Emb
4338 if (mod.comp.whole_cache_manifest) |man| {4336 if (mod.comp.whole_cache_manifest) |man| {
4339 const copied_resolved_path = try gpa.dupe(u8, resolved_path);4337 const copied_resolved_path = try gpa.dupe(u8, resolved_path);
4340 errdefer gpa.free(copied_resolved_path);4338 errdefer gpa.free(copied_resolved_path);
4341 mod.comp.whole_cache_manifest_mutex.lock();
4342 defer mod.comp.whole_cache_manifest_mutex.unlock();
4343 try man.addFilePostContents(copied_resolved_path, bytes, stat);4339 try man.addFilePostContents(copied_resolved_path, bytes, stat);
4344 }4340 }
43454341
src/stage1.zig+4-5
...@@ -455,11 +455,10 @@ export fn stage2_fetch_file(...@@ -455,11 +455,10 @@ export fn stage2_fetch_file(
455 const comp = @intToPtr(*Compilation, stage1.userdata);455 const comp = @intToPtr(*Compilation, stage1.userdata);
456 const file_path = path_ptr[0..path_len];456 const file_path = path_ptr[0..path_len];
457 const max_file_size = std.math.maxInt(u32);457 const max_file_size = std.math.maxInt(u32);
458 const contents = if (comp.whole_cache_manifest) |man| blk: {458 const contents = if (comp.whole_cache_manifest) |man|
459 comp.whole_cache_manifest_mutex.lock();459 man.addFilePostFetch(file_path, max_file_size) catch return null
460 defer comp.whole_cache_manifest_mutex.unlock();460 else
461 break :blk man.addFilePostFetch(file_path, max_file_size) catch return null;461 std.fs.cwd().readFileAlloc(comp.gpa, file_path, max_file_size) catch return null;
462 } else std.fs.cwd().readFileAlloc(comp.gpa, file_path, max_file_size) catch return null;
463 result_len.* = contents.len;462 result_len.* = contents.len;
464 // TODO https://github.com/ziglang/zig/issues/3328#issuecomment-716749475463 // TODO https://github.com/ziglang/zig/issues/3328#issuecomment-716749475
465 if (contents.len == 0) return @intToPtr(?[*]const u8, 0x1);464 if (contents.len == 0) return @intToPtr(?[*]const u8, 0x1);