Reduce the number of calls to the filesystem to see if a module file exists #14582

schveiguy · 2022-10-21T20:00:49Z

In a work project, this reduces the number of stat calls from 127_416 to 9_909.

This works by short-circuiting the stat calls if the parent directory is known not to exist.

With current dmd and this project, due to the -I parameters for each dependency (of which there are many), importing one module from phobos would result in 335 stat calls.

exists.

dlang-bot · 2022-10-21T20:00:51Z

Thanks for your pull request, @schveiguy!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14582"

schveiguy · 2022-10-21T20:08:26Z

compiler/src/dmd/file_manager.d

@@ -139,7 +234,6 @@ nothrow:
                }
                FileName.free(n2.ptr);
            }
-            FileName.free(n.ptr);


Note, I moved this to a scope(exit) on line 217 because it was a memory leak if the package.d file was found.

compiler/src/dmd/file_manager.d

nordlow · 2022-10-21T21:15:00Z

Why do we need to look at all the parent paths rather than just bottom-most parent?

schveiguy · 2022-10-21T21:19:16Z

Top-most parent might exist, even though the module isn't there. e.g. https://github.com/MartinNowak/io/tree/master/src/std/io

What we really need is to determine if any parent doesn't exist.

nordlow · 2022-10-21T21:21:24Z

Sorry, I don't understand this without a use case.

Do you want to cache calls to directory scans?

schveiguy · 2022-10-21T21:32:59Z

So if project a has source modules in:

projecta/source/foo/a

and project b has source modules in:

projectb/source/foo/b

and you import foo.b.bar, then this will check in order:

1. is projecta/foo/b cached? No
2. is projecta/foo cached? No
3. is projecta cached? No
4. is projecta a directory? Yes, cache it (stat)
5. is projecta/foo a directory? Yes, cache it (stat)
6. is projecta/foo/b a directory?  No -> cache and stop (stat)

7.  is projectb/foo/b cached? No
8.  is projectb/foo cached? No
9.  is projectb cached? No
10. is projectb a directory? Yes, cache it (stat)
11. is projectb/foo a directory? Yes, cache it (stat)
12. is projectb/foo/b a directory?  Yes -> cache and start looking for module. (6 stats)

Your proposal would have it just do:

1. is projecta/foo cached? No
2. is projecta/foo a directory? Yes, cache it and start looking for module (6 stats)

3. is projectb/foo cached? No
4. is projectb/foo a directory? Yes, cache it and start looking for module (6 stats)

Less steps, but the stats are more (12 vs 11).

Think about vibe.d, which has all its projects start with vibe.xxx, those would see no benefit from the caching.

schveiguy · 2022-10-21T21:33:59Z

Do you want to cache calls to directory scans?

There are no directory scans here.

compiler/src/dmd/file_manager.d

maxhaton · 2022-10-21T23:03:06Z

@schveiguy I retriggered failing CircleCI

nordlow · 2022-10-22T07:44:05Z

compiler/src/dmd/file_manager.d

+            }
+        }
+        // found a parent that is cached (or reached the end of the stack).
+        // step 2, traverse back up the stack storing either false if the


traverse back up

This should be

"traverse back down"

right?

Yeah could be. Not really great terminology.

nordlow · 2022-10-22T07:47:27Z

With current dmd and this project, due to the -I parameters for each dependency (of which there are many), importing one module from phobos would result in 335 stat calls.

I guess it would be value to the reviewers of this PR documenting how much this number drop when compiling Phobos with the branch of this PR.

compiler/src/dmd/file_manager.d

nordlow · 2022-10-29T20:27:16Z

Great work getting this merged so quickly.

On Ubuntu 22.10, I just tested dmd master as

echo > app.d && strace -e newfstatat dmd -c app.d -o-

at it seems there's still some redundant stats of .d files happening as show in the output

...
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/phobos/core", 0x7ffce054b990, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core", {st_mode=S_IFDIR|0775, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/attribute.di", 0x7ffce054bc90, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/attribute.d", {st_mode=S_IFREG|0664, st_size=8456, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/attribute.d", {st_mode=S_IFREG|0664, st_size=8456, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=8456, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal", {st_mode=S_IFDIR|0775, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/hash.di", 0x7ffce054bc90, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/hash.d", {st_mode=S_IFREG|0664, st_size=28155, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/hash.d", {st_mode=S_IFREG|0664, st_size=28155, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=28155, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/traits.di", 0x7ffce054bbe0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/traits.d", {st_mode=S_IFREG|0664, st_size=25079, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/traits.d", {st_mode=S_IFREG|0664, st_size=25079, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=25079, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/entrypoint.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/entrypoint.d", {st_mode=S_IFREG|0664, st_size=1155, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/entrypoint.d", {st_mode=S_IFREG|0664, st_size=1155, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=1155, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array", {st_mode=S_IFDIR|0775, st_size=4096, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/appending.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/appending.d", {st_mode=S_IFREG|0664, st_size=6055, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/appending.d", {st_mode=S_IFREG|0664, st_size=6055, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=6055, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/comparison.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/comparison.d", {st_mode=S_IFREG|0664, st_size=6812, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/comparison.d", {st_mode=S_IFREG|0664, st_size=6812, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=6812, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/equality.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/equality.d", {st_mode=S_IFREG|0664, st_size=7478, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/equality.d", {st_mode=S_IFREG|0664, st_size=7478, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=7478, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/casting.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/casting.d", {st_mode=S_IFREG|0664, st_size=4048, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/casting.d", {st_mode=S_IFREG|0664, st_size=4048, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=4048, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/concatenation.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/concatenation.d", {st_mode=S_IFREG|0664, st_size=2825, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/concatenation.d", {st_mode=S_IFREG|0664, st_size=2825, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=2825, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/construction.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/construction.d", {st_mode=S_IFREG|0664, st_size=7811, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/construction.d", {st_mode=S_IFREG|0664, st_size=7811, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=7811, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/arrayassign.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/arrayassign.d", {st_mode=S_IFREG|0664, st_size=9662, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/arrayassign.d", {st_mode=S_IFREG|0664, st_size=9662, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=9662, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/capacity.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/capacity.d", {st_mode=S_IFREG|0664, st_size=3066, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/array/capacity.d", {st_mode=S_IFREG|0664, st_size=3066, ...}, 0) = 0
newfstatat(3, "", {st_mode=S_IFREG|0664, st_size=3066, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/dassert.di", 0x7ffce054bcb0, 0) = -1 ENOENT (No such file or directory)
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/dassert.d", {st_mode=S_IFREG|0664, st_size=17769, ...}, 0) = 0
newfstatat(AT_FDCWD, "/home/per/.local/dlang/linux/bin64/src/druntime/import/core/internal/dassert.d", {st_mode=S_IFREG|0664, st_size=17769, ...}, 0) = 0
...

. In this run, Every .d under /home/per/.local/dlang/linux/bin64/src/druntime/import file is checked twice, @schveiguy.

schveiguy · 2022-10-31T14:45:28Z

.d under /home/per/.local/dlang/linux/bin64/src/druntime/import file is checked twice

I think that's the part that's reading the file data, not from this PR. Was it happening before this change?

The checks for the empty path also don't seem to be reasonable, but I don't know where those come from either.

nordlow · 2022-11-02T12:20:02Z

.d under /home/per/.local/dlang/linux/bin64/src/druntime/import file is checked twice

I think that's the part that's reading the file data, not from this PR. Was it happening before this change?

The checks for the empty path also don't seem to be reasonable, but I don't know where those come from either.

You can have gdb stop on a system call using catchpoints. For details see

According to the docs there is no way to filter on specific (string) arguments passed to the syscall, though. That would be very convenient in this case. Do you know of a way to accomplish such filtering, @ibuclaw?

Reduce the number of calls to the filesystem to see if a module file

0cbaad3

exists.

schveiguy commented Oct 21, 2022

View reviewed changes

schveiguy mentioned this pull request Oct 21, 2022

Fix 17699 - Importing a module that has both modulename.d and modulename/package.d should be an error #14407

Open

nordlow reviewed Oct 21, 2022

View reviewed changes

compiler/src/dmd/file_manager.d Show resolved Hide resolved

compiler/src/dmd/file_manager.d Show resolved Hide resolved

maxhaton reviewed Oct 21, 2022

View reviewed changes

compiler/src/dmd/file_manager.d Show resolved Hide resolved

nordlow reviewed Oct 22, 2022

View reviewed changes

dkorpel reviewed Oct 22, 2022

View reviewed changes

compiler/src/dmd/file_manager.d Show resolved Hide resolved

RazvanN7 approved these changes Oct 24, 2022

View reviewed changes

dkorpel approved these changes Oct 27, 2022

View reviewed changes

dkorpel added the Merge:auto-merge label Oct 27, 2022

dlang-bot merged commit a2865d7 into dlang:master Oct 27, 2022

schveiguy deleted the lowerstatcount branch October 27, 2022 17:01

schveiguy mentioned this pull request Feb 15, 2023

reapply fix Issue 23479 - ImportC recognizes .i and .c files, but not .h files #14864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the number of calls to the filesystem to see if a module file exists #14582

Reduce the number of calls to the filesystem to see if a module file exists #14582

schveiguy commented Oct 21, 2022

dlang-bot commented Oct 21, 2022

schveiguy Oct 21, 2022

nordlow commented Oct 21, 2022 •

edited

Loading

schveiguy commented Oct 21, 2022

nordlow commented Oct 21, 2022 •

edited

Loading

schveiguy commented Oct 21, 2022 •

edited

Loading

schveiguy commented Oct 21, 2022

maxhaton commented Oct 21, 2022

nordlow Oct 22, 2022 •

edited

Loading

schveiguy Oct 22, 2022

nordlow commented Oct 22, 2022 •

edited

Loading

nordlow commented Oct 29, 2022 •

edited

Loading

schveiguy commented Oct 31, 2022 •

edited

Loading

nordlow commented Nov 2, 2022 •

edited

Loading

Reduce the number of calls to the filesystem to see if a module file exists #14582

Reduce the number of calls to the filesystem to see if a module file exists #14582

Conversation

schveiguy commented Oct 21, 2022

dlang-bot commented Oct 21, 2022

Bugzilla references

Testing this PR locally

schveiguy Oct 21, 2022

Choose a reason for hiding this comment

nordlow commented Oct 21, 2022 • edited Loading

schveiguy commented Oct 21, 2022

nordlow commented Oct 21, 2022 • edited Loading

schveiguy commented Oct 21, 2022 • edited Loading

schveiguy commented Oct 21, 2022

maxhaton commented Oct 21, 2022

nordlow Oct 22, 2022 • edited Loading

Choose a reason for hiding this comment

schveiguy Oct 22, 2022

Choose a reason for hiding this comment

nordlow commented Oct 22, 2022 • edited Loading

nordlow commented Oct 29, 2022 • edited Loading

schveiguy commented Oct 31, 2022 • edited Loading

nordlow commented Nov 2, 2022 • edited Loading

nordlow commented Oct 21, 2022 •

edited

Loading

nordlow commented Oct 21, 2022 •

edited

Loading

schveiguy commented Oct 21, 2022 •

edited

Loading

nordlow Oct 22, 2022 •

edited

Loading

nordlow commented Oct 22, 2022 •

edited

Loading

nordlow commented Oct 29, 2022 •

edited

Loading

schveiguy commented Oct 31, 2022 •

edited

Loading

nordlow commented Nov 2, 2022 •

edited

Loading