#12474: std.fs.Dir.makeOpenPath: optimize case, if path already exists #14833

QusaiHroub · 2023-03-07T15:34:31Z

Uses a single NtCreateFile syscall on windows, falling back if the parent directory does not exist.

Closes #12474. Thanks to @joedavis and @matu3ba.

squeek502 · 2023-03-10T01:27:39Z

lib/std/fs.zig

-    fn openDirAccessMaskW(self: Dir, sub_path_w: [*:0]const u16, access_mask: u32, no_follow: bool) OpenError!Dir {
+    const OpenDirWithAccessAndCreationWOptions = struct {
+        no_follow: bool,
+        createDisposition: u32


Suggested change

createDisposition: u32

create_disposition: u32

https://ziglang.org/documentation/master/#Names

@squeek502 Thank you for the feedback, I have updated the PR to match your suggestion.

QusaiHroub · 2023-03-10T15:49:23Z

@Vexu Linux jobs are failing due to a formatting issue, I've updated the PR to fix that.

squeek502 · 2023-03-14T21:20:49Z

lib/std/fs.zig

+            const sub_path_w = try w.sliceToPrefixedFileW(sub_path[0..end_index]);
+            const result = self.openDirWithAccessAndCreationW(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;
+            }


It's not obvious to me what this logic is intending, or why it's necessary. Some explanatory comments would be nice, and a new test case that exercises this code added to std/fs/test.zig would be even better (ideally a test case that would fail if this logic wasn't included).

squeek502 · 2023-03-14T21:25:01Z

lib/std/fs.zig

+                return self.openDir(sub_path, open_dir_options) catch {
+                    try self.makePath(sub_path);
+                    return self.openDir(sub_path, open_dir_options);
+                };


I think switching on the caught error might make sense here, since for some errors it wouldn't make sense to makePath and retry. I think actually it might make sense to only retry on error.FileNotFound. Something like:

Suggested change

return self.openDir(sub_path, open_dir_options) catch {

try self.makePath(sub_path);

return self.openDir(sub_path, open_dir_options);

};

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,

};

(and same goes for makeOpenPathIterable)

squeek502 · 2023-03-19T11:58:42Z

lib/std/fs.zig

+    /// 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 openDirAccessMaskW(self: Dir, sub_path: []const u8, access_mask: u32, no_follow: bool) OpenError!Dir {


In light of the doc comments, I think a better name for this function would be something like makeOpenPathAccessMaskW.

andrewrk

The only issues I see here are doc comment issues, which I can take care of for you.

@squeek502 does this look OK to you? If so, I'll fix up the doc comments while merging.

andrewrk · 2023-06-18T23:26:01Z

lib/std/fs.zig

    /// 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 preforms `makeOpenPathAccessMaskW`.


This is not useful documentation since makeOpenPathAccessMaskW is (appropriately) private. The behavior of makeOpenPath must be fully documented here.

andrewrk · 2023-06-18T23:29:55Z

lib/std/fs.zig

@@ -1490,22 +1490,91 @@ pub const Dir = struct {
        }
    }

+    /// Calls makeOpenDirAccessMaskW recursively to make an entire path


Please avoid recursion in this function. Recursion is problematic because it has unbounded stack usage that potentially depends on user input. It also makes the compiler unable to perform many kinds of optimizations.

Related: #1006

It does not look like you actually used recursion here, so perhaps the only change needed is that this doc comment is incorrect.

Thank you for the feedback!
Yes, It isn't an actual recursion. The call stack size is O(1) and doesn't depend on the user input.
Could you please take care of the incorrect doc comment?

@andrewrk note that this 'recursively' language matches the doc comment of makePath above which shares the same logic.

squeek502 · 2023-06-19T00:36:09Z

lib/std/fs/test.zig

+test "makeOpenPath parent dirs do not exist" {
+    if (builtin.os.tag == .wasi) return error.SkipZigTest;
+
+    var dir = try fs.cwd().makeOpenPath("root_dir/parent_dir/some_dir", .{});
+    defer dir.close();
+}


This test should use std.testing.tmpDir to avoid polluting the cwd (and to allow it to work when targeting WASI). Might also be good to double check that the full path was created (this is a bit redundant since in theory makeOpenPath returning a Dir means that the directory was created, but the double checking would potentially catch a bug where e.g. only ./some_dir is created and its fd returned when passed a path like ./root_dir/parent_dir/some_dir).

Something like this (untested):

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(); }

Thank you for the feedback!
I have updated the PR and reflected your suggestion.

…case See this discussion ziglang#14833 (comment) for more Info

andrewrk · 2023-06-19T18:45:33Z

@QusaiHroub would you mind rebasing so that there is not a merge commit in this PR?

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub · 2023-06-19T19:04:46Z

@QusaiHroub would you mind rebasing so that there is not a merge commit in this PR?

Done

andrewrk · 2023-06-19T19:17:50Z

Thanks!

…case See this discussion ziglang#14833 (comment) for more Info

squeek502 · 2023-07-27T21:03:19Z

Since #16570 has been merged, this can also usepath.ComponentIterator.

I've made all the necessary changes in my fork here: master...squeek502:zig:optimize_std.fs.Dir.makeOpenPath_12474

as well as a few other fixes (fixed leaking intermediate dir handles, reworded the doc comments)

There are a few ways to go here:

You can rebase your branch on master and cherry pick the commits from my fork
You can rebase your branch on master and I can submit my changes as a pull request to your branch
I can submit my branch as a new pull request

Let me know what you'd like to do.

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub · 2023-07-28T05:28:30Z

Since #16570 has been merged, this can also usepath.ComponentIterator.

I've made all the necessary changes in my fork here: master...squeek502:zig:optimize_std.fs.Dir.makeOpenPath_12474

as well as a few other fixes (fixed leaking intermediate dir handles, reworded the doc comments)

There are a few ways to go here:

You can rebase your branch on master and cherry pick the commits from my fork

You can rebase your branch on master and I can submit my changes as a pull request to your branch

I can submit my branch as a new pull request

Let me know what you'd like to do.

Thank you for your work, I chose to go the first way, and I did.

lib/std/fs.zig

…case See this discussion ziglang#14833 (comment) for more Info

@joedavis

Uses a single NtCreateFile syscall on windows. Closes ziglang#12474. Thanks to @joedavis and @matu3ba.

See 49053cb for details Also, fix leaking the intermediate directory handles.

These are not recursive functions, so 'recursively' could be misleading.

squeek502 · 2023-10-22T05:41:04Z

Thanks for working on this @QusaiHroub!

squeek502 reviewed Mar 10, 2023

View reviewed changes

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch 7 times, most recently from 6e6032e to d26f53f Compare March 10, 2023 15:42

QusaiHroub requested a review from squeek502 March 14, 2023 17:02

squeek502 reviewed Mar 14, 2023

View reviewed changes

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 6556eec to 00acf66 Compare March 17, 2023 13:48

squeek502 reviewed Mar 19, 2023

View reviewed changes

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch 3 times, most recently from 63a666c to 8da822d Compare March 24, 2023 14:29

andrewrk force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 8da822d to 835ee98 Compare June 18, 2023 08:45

andrewrk requested changes Jun 18, 2023

View reviewed changes

squeek502 reviewed Jun 19, 2023

View reviewed changes

QusaiHroub added a commit to QusaiHroub/zig that referenced this pull request Jun 19, 2023

[ziglang#12474]: Update 'makeOpenPath parent dirs do not exist' test …

4702da4

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub added a commit to QusaiHroub/zig that referenced this pull request Jun 19, 2023

[ziglang#12474]: Update 'makeOpenPath parent dirs do not exist' test …

d2ef663

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 4702da4 to d2ef663 Compare June 19, 2023 19:00

squeek502 mentioned this pull request Jul 26, 2023

Add fs.path.ComponentIterator and use it in Dir.makePath #16570

Merged

squeek502 pushed a commit to squeek502/zig that referenced this pull request Jul 27, 2023

[ziglang#12474]: Update 'makeOpenPath parent dirs do not exist' test …

bb3651e

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub added a commit to QusaiHroub/zig that referenced this pull request Jul 28, 2023

[ziglang#12474]: Update 'makeOpenPath parent dirs do not exist' test …

16000ae

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from d2ef663 to 16000ae Compare July 28, 2023 04:57

matu3ba reviewed Jul 28, 2023

View reviewed changes

lib/std/fs.zig Outdated Show resolved Hide resolved

squeek502 approved these changes Aug 2, 2023

View reviewed changes

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 23fff77 to 364f68f Compare August 3, 2023 05:57

QusaiHroub requested review from kprotty and kristoff-it as code owners August 3, 2023 05:57

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from be6432d to 364f68f Compare August 3, 2023 05:59

QusaiHroub added a commit to QusaiHroub/zig that referenced this pull request Aug 3, 2023

[ziglang#12474]: Update 'makeOpenPath parent dirs do not exist' test …

cc6457b

…case See this discussion ziglang#14833 (comment) for more Info

QusaiHroub force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 364f68f to 0363c52 Compare August 3, 2023 06:01

QusaiHroub and others added 2 commits October 18, 2023 20:36

std.fs.Dir.makeOpenPath: optimize when path already exists

f6a6cdb

Uses a single NtCreateFile syscall on windows. Closes ziglang#12474. Thanks to @joedavis and @matu3ba.

path.ComponentIterator: Add peekNext and peekPrevious functions

c139b9d

andrewrk force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 0363c52 to 595f1f1 Compare October 19, 2023 03:43

andrewrk enabled auto-merge October 19, 2023 03:43

squeek502 added 2 commits October 20, 2023 23:25

Dir.makeOpenPathAccessMaskW: Use path.ComponentIterator

63b5042

See 49053cb for details Also, fix leaking the intermediate directory handles.

Improve Dir.makePath and Dir.makeOpenPathAccessMaskW doc comments

fb5f69a

These are not recursive functions, so 'recursively' could be misleading.

squeek502 force-pushed the optimize_std.fs.Dir.makeOpenPath_12474 branch from 595f1f1 to fb5f69a Compare October 21, 2023 06:26

andrewrk merged commit e354aac into ziglang:master Oct 22, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#12474: std.fs.Dir.makeOpenPath: optimize case, if path already exists #14833

#12474: std.fs.Dir.makeOpenPath: optimize case, if path already exists #14833

QusaiHroub commented Mar 7, 2023

squeek502 Mar 10, 2023

QusaiHroub Mar 10, 2023

QusaiHroub commented Mar 10, 2023

squeek502 Mar 14, 2023 •

edited

Loading

squeek502 Mar 14, 2023 •

edited

Loading

squeek502 Mar 19, 2023

QusaiHroub Mar 21, 2023

andrewrk left a comment

andrewrk Jun 18, 2023

andrewrk Jun 18, 2023

QusaiHroub Jun 19, 2023

squeek502 Jun 20, 2023

squeek502 Jun 19, 2023 •

edited

Loading

QusaiHroub Jun 19, 2023

andrewrk commented Jun 19, 2023

QusaiHroub commented Jun 19, 2023

andrewrk commented Jun 19, 2023

squeek502 commented Jul 27, 2023

QusaiHroub commented Jul 28, 2023

squeek502 commented Oct 22, 2023

#12474: std.fs.Dir.makeOpenPath: optimize case, if path already exists #14833

#12474: std.fs.Dir.makeOpenPath: optimize case, if path already exists #14833

Conversation

QusaiHroub commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QusaiHroub commented Mar 10, 2023

squeek502 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

squeek502 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeek502 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk commented Jun 19, 2023

QusaiHroub commented Jun 19, 2023

andrewrk commented Jun 19, 2023

squeek502 commented Jul 27, 2023

QusaiHroub commented Jul 28, 2023

squeek502 commented Oct 22, 2023

squeek502 Mar 14, 2023 •

edited

Loading

squeek502 Mar 14, 2023 •

edited

Loading

squeek502 Jun 19, 2023 •

edited

Loading