From f6a6cdbba372414e441bbcea8c680e71d12c4a9a Mon Sep 17 00:00:00 2001 From: Qusai Hroub Date: Sat, 4 Mar 2023 13:10:33 +0200 Subject: [PATCH 1/4] std.fs.Dir.makeOpenPath: optimize when path already exists Uses a single NtCreateFile syscall on windows. Closes #12474. Thanks to @joedavis and @matu3ba. --- lib/std/fs.zig | 100 +++++++++++++++++++++++++++++++++++++++----- lib/std/fs/test.zig | 12 ++++++ 2 files changed, 101 insertions(+), 11 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index d95321cfd3..808124f5aa 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1483,22 +1483,91 @@ pub const Dir = struct { } } + /// Calls makeOpenDirAccessMaskW recursively to make an entire path + /// (i.e. falling back if the parent directory does not exist). Opens the dir if the path + /// already exists and is a directory. + /// This function is not atomic, and if it returns an error, the file system may + /// have been modified regardless. + fn makeOpenPathAccessMaskW(self: Dir, sub_path: []const u8, access_mask: u32, no_follow: bool) OpenError!Dir { + const w = os.windows; + var end_index: usize = sub_path.len; + + return while (true) { + const sub_path_w = try w.sliceToPrefixedFileW(sub_path[0..end_index]); + const result = self.makeOpenDirAccessMaskW(sub_path_w.span().ptr, access_mask, .{ + .no_follow = no_follow, + .create_disposition = if (end_index == sub_path.len) w.FILE_OPEN_IF else w.FILE_CREATE, + }) catch |err| switch (err) { + error.FileNotFound => { + // march end_index backward until next path component + while (true) { + if (end_index == 0) return err; + end_index -= 1; + if (path.isSep(sub_path[end_index])) break; + } + continue; + }, + else => return err, + }; + + if (end_index == sub_path.len) return result; + // march end_index forward until next path component + while (true) { + end_index += 1; + if (end_index == sub_path.len or path.isSep(sub_path[end_index])) break; + } + }; + } + /// This function performs `makePath`, followed by `openDir`. /// If supported by the OS, this operation is atomic. It is not atomic on /// all operating systems. + /// On Windows, this function performs `makeOpenPathAccessMaskW`. pub fn makeOpenPath(self: Dir, sub_path: []const u8, open_dir_options: OpenDirOptions) !Dir { - // TODO improve this implementation on Windows; we can avoid 1 call to NtClose - try self.makePath(sub_path); - return self.openDir(sub_path, open_dir_options); + return switch (builtin.os.tag) { + .windows => { + const w = os.windows; + const base_flags = w.STANDARD_RIGHTS_READ | w.FILE_READ_ATTRIBUTES | w.FILE_READ_EA | + w.SYNCHRONIZE | w.FILE_TRAVERSE; + + return self.makeOpenPathAccessMaskW(sub_path, base_flags, open_dir_options.no_follow); + }, + else => { + return self.openDir(sub_path, open_dir_options) catch |err| switch (err) { + error.FileNotFound => { + try self.makePath(sub_path); + return self.openDir(sub_path, open_dir_options); + }, + else => |e| return e, + }; + }, + }; } /// This function performs `makePath`, followed by `openIterableDir`. /// If supported by the OS, this operation is atomic. It is not atomic on /// all operating systems. pub fn makeOpenPathIterable(self: Dir, sub_path: []const u8, open_dir_options: OpenDirOptions) !IterableDir { - // TODO improve this implementation on Windows; we can avoid 1 call to NtClose - try self.makePath(sub_path); - return self.openIterableDir(sub_path, open_dir_options); + return switch (builtin.os.tag) { + .windows => { + const w = os.windows; + const base_flags = w.STANDARD_RIGHTS_READ | w.FILE_READ_ATTRIBUTES | w.FILE_READ_EA | + w.SYNCHRONIZE | w.FILE_TRAVERSE | w.FILE_LIST_DIRECTORY; + + return IterableDir{ + .dir = try self.makeOpenPathAccessMaskW(sub_path, base_flags, open_dir_options.no_follow), + }; + }, + else => { + return self.openIterableDir(sub_path, open_dir_options) catch |err| switch (err) { + error.FileNotFound => { + try self.makePath(sub_path); + return self.openIterableDir(sub_path, open_dir_options); + }, + else => |e| return e, + }; + }, + }; } /// This function returns the canonicalized absolute pathname of @@ -1742,7 +1811,10 @@ pub const Dir = struct { const base_flags = w.STANDARD_RIGHTS_READ | w.FILE_READ_ATTRIBUTES | w.FILE_READ_EA | w.SYNCHRONIZE | w.FILE_TRAVERSE; const flags: u32 = if (iterable) base_flags | w.FILE_LIST_DIRECTORY else base_flags; - var dir = try self.openDirAccessMaskW(sub_path_w, flags, args.no_follow); + var dir = try self.makeOpenDirAccessMaskW(sub_path_w, flags, .{ + .no_follow = args.no_follow, + .create_disposition = w.FILE_OPEN, + }); return dir; } @@ -1765,7 +1837,12 @@ pub const Dir = struct { return Dir{ .fd = fd }; } - fn openDirAccessMaskW(self: Dir, sub_path_w: [*:0]const u16, access_mask: u32, no_follow: bool) OpenError!Dir { + const MakeOpenDirAccessMaskWOptions = struct { + no_follow: bool, + create_disposition: u32, + }; + + fn makeOpenDirAccessMaskW(self: Dir, sub_path_w: [*:0]const u16, access_mask: u32, flags: MakeOpenDirAccessMaskWOptions) OpenError!Dir { const w = os.windows; var result = Dir{ @@ -1786,7 +1863,7 @@ pub const Dir = struct { .SecurityDescriptor = null, .SecurityQualityOfService = null, }; - const open_reparse_point: w.DWORD = if (no_follow) w.FILE_OPEN_REPARSE_POINT else 0x0; + const open_reparse_point: w.DWORD = if (flags.no_follow) w.FILE_OPEN_REPARSE_POINT else 0x0; var io: w.IO_STATUS_BLOCK = undefined; const rc = w.ntdll.NtCreateFile( &result.fd, @@ -1794,13 +1871,14 @@ pub const Dir = struct { &attr, &io, null, - 0, + w.FILE_ATTRIBUTE_NORMAL, w.FILE_SHARE_READ | w.FILE_SHARE_WRITE, - w.FILE_OPEN, + flags.create_disposition, w.FILE_DIRECTORY_FILE | w.FILE_SYNCHRONOUS_IO_NONALERT | w.FILE_OPEN_FOR_BACKUP_INTENT | open_reparse_point, null, 0, ); + switch (rc) { .SUCCESS => return result, .OBJECT_NAME_INVALID => return error.BadPathName, diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 6f607bfa45..9a11c7cb29 100644 --- a/lib/std/fs/test.zig +++ b/lib/std/fs/test.zig @@ -663,6 +663,18 @@ test "file operations on directories" { }.impl); } +test "makeOpenPath parent dirs do not exist" { + var tmp_dir = tmpDir(.{}); + defer tmp_dir.cleanup(); + + var dir = try tmp_dir.dir.makeOpenPath("root_dir/parent_dir/some_dir", .{}); + dir.close(); + + // double check that the full directory structure was created + var dir_verification = try tmp_dir.dir.openDir("root_dir/parent_dir/some_dir", .{}); + dir_verification.close(); +} + test "deleteDir" { try testWithAllSupportedPathTypes(struct { fn impl(ctx: *TestContext) !void { From c139b9d4ad68d1cb5e2c489891493232a11db76c Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Thu, 27 Jul 2023 12:57:19 -0700 Subject: [PATCH 2/4] path.ComponentIterator: Add peekNext and peekPrevious functions --- lib/std/fs/path.zig | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/std/fs/path.zig b/lib/std/fs/path.zig index 8457991f59..395b5a335d 100644 --- a/lib/std/fs/path.zig +++ b/lib/std/fs/path.zig @@ -1507,6 +1507,14 @@ pub fn ComponentIterator(comptime path_type: PathType, comptime T: type) type { /// For example, if the path is `/a/b/c` and the most recently returned component /// is `b`, then this will return the `c` component. pub fn next(self: *Self) ?Component { + const peek_result = self.peekNext() orelse return null; + self.start_index = peek_result.path.len - peek_result.name.len; + self.end_index = peek_result.path.len; + return peek_result; + } + + /// Like `next`, but does not modify the iterator state. + pub fn peekNext(self: Self) ?Component { var start_index = self.end_index; while (start_index < self.path.len and path_type.isSep(T, self.path[start_index])) { start_index += 1; @@ -1516,11 +1524,9 @@ pub fn ComponentIterator(comptime path_type: PathType, comptime T: type) type { end_index += 1; } if (start_index == end_index) return null; - self.start_index = start_index; - self.end_index = end_index; return .{ - .name = self.path[self.start_index..self.end_index], - .path = self.path[0..self.end_index], + .name = self.path[start_index..end_index], + .path = self.path[0..end_index], }; } @@ -1529,6 +1535,14 @@ pub fn ComponentIterator(comptime path_type: PathType, comptime T: type) type { /// For example, if the path is `/a/b/c` and the most recently returned component /// is `b`, then this will return the `a` component. pub fn previous(self: *Self) ?Component { + const peek_result = self.peekPrevious() orelse return null; + self.start_index = peek_result.path.len - peek_result.name.len; + self.end_index = peek_result.path.len; + return peek_result; + } + + /// Like `previous`, but does not modify the iterator state. + pub fn peekPrevious(self: Self) ?Component { var end_index = self.start_index; while (true) { if (end_index == self.root_end_index) return null; @@ -1542,11 +1556,9 @@ pub fn ComponentIterator(comptime path_type: PathType, comptime T: type) type { start_index -= 1; } if (start_index == end_index) return null; - self.start_index = start_index; - self.end_index = end_index; return .{ - .name = self.path[self.start_index..self.end_index], - .path = self.path[0..self.end_index], + .name = self.path[start_index..end_index], + .path = self.path[0..end_index], }; } }; From 63b504219d119a13731811065d18a2b10b4214b3 Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Thu, 27 Jul 2023 12:59:30 -0700 Subject: [PATCH 3/4] Dir.makeOpenPathAccessMaskW: Use path.ComponentIterator See 49053cb1b4af7ee2973cf606def398c7eef6578b for details Also, fix leaking the intermediate directory handles. --- lib/std/fs.zig | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 808124f5aa..329f5ae164 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1490,33 +1490,31 @@ pub const Dir = struct { /// have been modified regardless. fn makeOpenPathAccessMaskW(self: Dir, sub_path: []const u8, access_mask: u32, no_follow: bool) OpenError!Dir { const w = os.windows; - var end_index: usize = sub_path.len; + var it = try path.componentIterator(sub_path); + // If there are no components in the path, then create a dummy component with the full path. + var component = it.last() orelse path.NativeUtf8ComponentIterator.Component{ + .name = "", + .path = sub_path, + }; - return while (true) { - const sub_path_w = try w.sliceToPrefixedFileW(sub_path[0..end_index]); - const result = self.makeOpenDirAccessMaskW(sub_path_w.span().ptr, access_mask, .{ + while (true) { + const sub_path_w = try w.sliceToPrefixedFileW(self.fd, component.path); + const is_last = it.peekNext() == null; + var result = self.makeOpenDirAccessMaskW(sub_path_w.span().ptr, access_mask, .{ .no_follow = no_follow, - .create_disposition = if (end_index == sub_path.len) w.FILE_OPEN_IF else w.FILE_CREATE, + .create_disposition = if (is_last) w.FILE_OPEN_IF else w.FILE_CREATE, }) catch |err| switch (err) { - error.FileNotFound => { - // march end_index backward until next path component - while (true) { - if (end_index == 0) return err; - end_index -= 1; - if (path.isSep(sub_path[end_index])) break; - } + error.FileNotFound => |e| { + component = it.previous() orelse return e; continue; }, - else => return err, + else => |e| return e, }; - if (end_index == sub_path.len) return result; - // march end_index forward until next path component - while (true) { - end_index += 1; - if (end_index == sub_path.len or path.isSep(sub_path[end_index])) break; - } - }; + component = it.next() orelse return result; + // Don't leak the intermediate file handles + result.close(); + } } /// This function performs `makePath`, followed by `openDir`. From fb5f69a55283e65a8ae9508e74b9eefedc3ac3da Mon Sep 17 00:00:00 2001 From: Ryan Liptak Date: Thu, 27 Jul 2023 13:57:41 -0700 Subject: [PATCH 4/4] Improve Dir.makePath and Dir.makeOpenPathAccessMaskW doc comments These are not recursive functions, so 'recursively' could be misleading. --- lib/std/fs.zig | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/std/fs.zig b/lib/std/fs.zig index 329f5ae164..43fb6ec0c8 100644 --- a/lib/std/fs.zig +++ b/lib/std/fs.zig @@ -1459,8 +1459,9 @@ pub const Dir = struct { try os.mkdiratW(self.fd, sub_path, default_new_dir_mode); } - /// Calls makeDir recursively to make an entire path. Returns success if the path - /// already exists and is a directory. + /// Calls makeDir iteratively to make an entire path + /// (i.e. creating any parent directories that do not exist). + /// Returns success if the path already exists and is a directory. /// This function is not atomic, and if it returns an error, the file system may /// have been modified regardless. pub fn makePath(self: Dir, sub_path: []const u8) !void { @@ -1483,9 +1484,9 @@ pub const Dir = struct { } } - /// Calls makeOpenDirAccessMaskW recursively to make an entire path - /// (i.e. falling back if the parent directory does not exist). Opens the dir if the path - /// already exists and is a directory. + /// Calls makeOpenDirAccessMaskW iteratively to make an entire path + /// (i.e. creating any parent directories that do not exist). + /// Opens the dir if the path already exists and is a directory. /// This function is not atomic, and if it returns an error, the file system may /// have been modified regardless. fn makeOpenPathAccessMaskW(self: Dir, sub_path: []const u8, access_mask: u32, no_follow: bool) OpenError!Dir {