Skip to content

Moved the check for formatting a directory#5648

Closed
DrDeano wants to merge 1 commit intoziglang:masterfrom
DrDeano:master
Closed

Moved the check for formatting a directory#5648
DrDeano wants to merge 1 commit intoziglang:masterfrom
DrDeano:master

Conversation

@DrDeano
Copy link
Contributor

@DrDeano DrDeano commented Jun 20, 2020

The original check for a directory was for the readAllAlloc so move the check from open to read. This in turn fixes the fmt step in the build script for directories.

The original check for a directory was for the `readAllAlloc` so move the check from open to read. This in turn fixes the fmt step in the build script for directories.
@DrDeano
Copy link
Contributor Author

DrDeano commented Jun 20, 2020

Closes ZystemOS/pluto#165

@squeek502
Copy link
Member

squeek502 commented Jun 20, 2020

I believe the catch needs to be in both open and read rather than moved from one to the other. If I'm reading the code correctly, then on Windows, error.IsDir can only happen during open.

(note that previously, the fs.cwd().readAllAlloc was doing open/stat/read all at once)

EDIT: Would fs.cwd().openFile not returning error.IsDir on Linux be considered a bug, by the way? I'm presuming that's what's going on.

@andrewrk
Copy link
Member

@DrDeano thank you for the patch, and apologies for the downstream breakage. This landed in edea7a4.

EDIT: Would fs.cwd().openFile not returning error.IsDir on Linux be considered a bug, by the way? I'm presuming that's what's going on.

On Linux, when opening a file with read-only permissions, there is no way to determine whether the file descriptor is a directory handle without an additional syscall. In the additional commits I added to this branch before merging, I modified the symlink loop detection to use the inode rather than the realpath. This means unconditionally doing a stat() for both directories and files, which also tells whether the file descriptor is a directory or not.

I got a bit carried away with the merge, but the end result should be that zig fmt is significantly faster with this merge than before. From the operating system's perspective, running zig fmt on an already formatted codebase now looks like (repeated for every file):

openat(4, "poly1305.zig", O_RDONLY|O_CLOEXEC) = 5
fstat(5, {st_mode=S_IFREG|0644, st_size=8356, ...}) = 0
read(5, "// Translated from monocypher wh"..., 8356) = 8356
close(5)                                = 0

and (repeated for every directory)

openat(3, "time", O_RDONLY|O_CLOEXEC|O_DIRECTORY) = 4
fstat(4, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
getdents64(4, /* 3 entries */, 8192)    = 80

@andrewrk
Copy link
Member

This auto generated graph from https://github.com/ziglang/gotta-go-fast/blob/master/records.csv is messy, but you can still see the dip at the end from the merge:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants