Skip to content

Commit

Permalink
Module: rewrite zir caching logic
Browse files Browse the repository at this point in the history 
Multiple processes can sit waiting for the exclusive lock at the same
time, so we want to recheck whether it needs to be updated whenever
we get an exclusive lock.

This also fixes a race condition between one process truncating the
cache file and another process reading it without atomic locking.
  • Loading branch information
@jacobly0 @andrewrk
jacobly0 authored and andrewrk committed Mar 8, 2023
1 parent 6fc1621 commit e3cf9d1
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 67 deletions.
127 changes: 62 additions & 65 deletions src/Module.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3538,44 +3538,61 @@ pub fn astGenFile(mod: *Module, file: *File) !void {
const cache_directory = if (want_local_cache) mod.local_zir_cache else mod.global_zir_cache;
const zir_dir = cache_directory.handle;

var cache_file: ?std.fs.File = null;
defer if (cache_file) |f| f.close();

// Determine whether we need to reload the file from disk and redo parsing and AstGen.
switch (file.status) {
.never_loaded, .retryable_failure => cached: {
var lock: std.fs.File.Lock = switch (file.status) {
.never_loaded, .retryable_failure => lock: {
// First, load the cached ZIR code, if any.
log.debug("AstGen checking cache: {s} (local={}, digest={s})", .{
file.sub_file_path, want_local_cache, &digest,
});

// We ask for a lock in order to coordinate with other zig processes.
// If another process is already working on this file, we will get the cached
// version. Likewise if we're working on AstGen and another process asks for
// the cached file, they'll get it.
cache_file = zir_dir.openFile(&digest, .{ .lock = .Shared }) catch |err| switch (err) {
error.PathAlreadyExists => unreachable, // opening for reading
error.NoSpaceLeft => unreachable, // opening for reading
error.NotDir => unreachable, // no dir components
error.InvalidUtf8 => unreachable, // it's a hex encoded name
error.BadPathName => unreachable, // it's a hex encoded name
error.NameTooLong => unreachable, // it's a fixed size name
error.PipeBusy => unreachable, // it's not a pipe
error.WouldBlock => unreachable, // not asking for non-blocking I/O

error.SymLinkLoop,
error.FileNotFound,
error.Unexpected,
=> break :cached,

else => |e| return e, // Retryable errors are handled at callsite.
};
break :lock .Shared;
},
.parse_failure, .astgen_failure, .success_zir => lock: {
const unchanged_metadata =
stat.size == file.stat.size and
stat.mtime == file.stat.mtime and
stat.inode == file.stat.inode;

if (unchanged_metadata) {
log.debug("unmodified metadata of file: {s}", .{file.sub_file_path});
return;
}

log.debug("metadata changed: {s}", .{file.sub_file_path});

break :lock .Exclusive;
},
};

// We ask for a lock in order to coordinate with other zig processes.
// If another process is already working on this file, we will get the cached
// version. Likewise if we're working on AstGen and another process asks for
// the cached file, they'll get it.
const cache_file = zir_dir.createFile(&digest, .{
.read = true,
.truncate = false,
.lock = lock,
}) catch |err| switch (err) {
error.NotDir => unreachable, // no dir components
error.InvalidUtf8 => unreachable, // it's a hex encoded name
error.BadPathName => unreachable, // it's a hex encoded name
error.NameTooLong => unreachable, // it's a fixed size name
error.PipeBusy => unreachable, // it's not a pipe
error.WouldBlock => unreachable, // not asking for non-blocking I/O
error.FileNotFound => unreachable, // no dir components

else => |e| return e, // Retryable errors are handled at callsite.
};
defer cache_file.close();

while (true) {
update: {
// First we read the header to determine the lengths of arrays.
const header = cache_file.?.reader().readStruct(Zir.Header) catch |err| switch (err) {
const header = cache_file.reader().readStruct(Zir.Header) catch |err| switch (err) {
// This can happen if Zig bails out of this function between creating
// the cached file and writing it.
error.EndOfStream => break :cached,
error.EndOfStream => break :update,
else => |e| return e,
};
const unchanged_metadata =
Expand All @@ -3585,7 +3602,7 @@ pub fn astGenFile(mod: *Module, file: *File) !void {

if (!unchanged_metadata) {
log.debug("AstGen cache stale: {s}", .{file.sub_file_path});
break :cached;
break :update;
}
log.debug("AstGen cache hit: {s} instructions_len={d}", .{
file.sub_file_path, header.instructions_len,
Expand Down Expand Up @@ -3637,13 +3654,13 @@ pub fn astGenFile(mod: *Module, file: *File) !void {
.iov_len = header.extra_len * 4,
},
};
const amt_read = try cache_file.?.readvAll(&iovecs);
const amt_read = try cache_file.readvAll(&iovecs);
const amt_expected = zir.instructions.len * 9 +
zir.string_bytes.len +
zir.extra.len * 4;
if (amt_read != amt_expected) {
log.warn("unexpected EOF reading cached ZIR for {s}", .{file.sub_file_path});
break :cached;
break :update;
}
if (data_has_safety_tag) {
const tags = zir.instructions.items(.tag);
Expand Down Expand Up @@ -3679,42 +3696,22 @@ pub fn astGenFile(mod: *Module, file: *File) !void {
return error.AnalysisFail;
}
return;
},
.parse_failure, .astgen_failure, .success_zir => {
const unchanged_metadata =
stat.size == file.stat.size and
stat.mtime == file.stat.mtime and
stat.inode == file.stat.inode;

if (unchanged_metadata) {
log.debug("unmodified metadata of file: {s}", .{file.sub_file_path});
return;
}
}

log.debug("metadata changed: {s}", .{file.sub_file_path});
},
}
if (cache_file) |f| {
f.close();
cache_file = null;
// If we already have the exclusive lock then it is our job to update.
if (builtin.os.tag == .wasi or lock == .Exclusive) break;
// Otherwise, unlock to give someone a chance to get the exclusive lock
// and then upgrade to an exclusive lock.
cache_file.unlock();
lock = .Exclusive;
try cache_file.lock(lock);
}
cache_file = zir_dir.createFile(&digest, .{ .lock = .Exclusive }) catch |err| switch (err) {
error.NotDir => unreachable, // no dir components
error.InvalidUtf8 => unreachable, // it's a hex encoded name
error.BadPathName => unreachable, // it's a hex encoded name
error.NameTooLong => unreachable, // it's a fixed size name
error.PipeBusy => unreachable, // it's not a pipe
error.WouldBlock => unreachable, // not asking for non-blocking I/O
error.FileNotFound => unreachable, // no dir components

else => |e| {
const pkg_path = file.pkg.root_src_directory.path orelse ".";
const cache_path = cache_directory.path orelse ".";
log.warn("unable to save cached ZIR code for {s}/{s} to {s}/{s}: {s}", .{
pkg_path, file.sub_file_path, cache_path, &digest, @errorName(e),
});
return;
},
// The cache is definitely stale so delete the contents to avoid an underwrite later.
cache_file.setEndPos(0) catch |err| switch (err) {
error.FileTooBig => unreachable, // 0 is not too big

else => |e| return e,
};

mod.lockAndClearFileCompileError(file);
Expand Down Expand Up @@ -3871,7 +3868,7 @@ pub fn astGenFile(mod: *Module, file: *File) !void {
.iov_len = file.zir.extra.len * 4,
},
};
cache_file.?.writevAll(&iovecs) catch |err| {
cache_file.writevAll(&iovecs) catch |err| {
const pkg_path = file.pkg.root_src_directory.path orelse ".";
const cache_path = cache_directory.path orelse ".";
log.warn("unable to write cached ZIR code for {s}/{s} to {s}/{s}: {s}", .{
Expand Down
2 changes: 0 additions & 2 deletions stage1/wasi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,6 @@ uint32_t wasi_snapshot_preview1_fd_read(uint32_t fd, uint32_t iovs, uint32_t iov
size_t read_size = 0;
if (fds[fd].stream != NULL)
read_size = fread(&m[iovs_ptr[i].ptr], 1, iovs_ptr[i].len, fds[fd].stream);
else
panic("unimplemented");
size += read_size;
if (read_size < iovs_ptr[i].len) break;
}
Expand Down

0 comments on commit e3cf9d1

Please sign in to comment.