Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CreateDirectory: eliminate some syscalls. #58799

Merged
merged 4 commits into from
Nov 18, 2021

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 8, 2021

This eliminates three syscalls per Directory.CreateDirectory when the path doesn't exist.

One is the initial check if the directory exist.

The other two are eliminating by using mkdir to check parent directory existence.
Instead of finding the first parent that exist using stat, we now end up creating the first parent that doesn't exist.

When Directory.CreateDirectory is called with a path that does exist, we are now making two syscalls instead of one.
If we want, we can avoid that regression by keeping the initial existence check. That costs us a syscall in the non-exists case.
A user can also avoid it by calling Directory.Exists before calling Directory.CreateDirectory.

Similar changes can be made to the Windows implementation.

@adamsitnik @stephentoub ptal.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO and removed community-contribution Indicates that the PR has been added by a community member labels Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

This eliminates three syscalls per Directory.CreateDirectory when the path doesn't exist.

One is the initial check if the directory exist.

The other two are eliminating by using mkdir to check parent directory existence.
Instead of finding the first parent that exist using stat, we now end up creating the first parent that doesn't exist.

When Directory.CreateDirectory is called with a path that does exist, we are now making two syscalls instead of one.
If we want, we can avoid that regression by keeping the initial existence check. That costs us a syscall in the non-exists case.
A user can also avoid it by calling Directory.Exists before calling Directory.CreateDirectory.

Similar changes can be made to the Windows implementation.

@adamsitnik @stephentoub ptal.

Author: tmds
Assignees: -
Labels:

area-System.IO

Milestone: -

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I'd like a secondary review from @dotnet/area-system-io.

@jeffhandley jeffhandley added the community-contribution Indicates that the PR has been added by a community member label Nov 15, 2021
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but it would be great to refactor the code a little bit and if it's safe replace List<string> with List<int>

@tmds thank you for another great contribution!

@tmds
Copy link
Member Author

tmds commented Nov 18, 2021

@adamsitnik thanks for the review! I've addressed your comments.
I kept the List<string> since I need a string to pass to the MkDir PInvoke.

@tmds tmds force-pushed the createdir_syscalls branch from 8832768 to 0a03e35 Compare November 18, 2021 06:05
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, again thank you @tmds !

@adamsitnik adamsitnik merged commit 37bf145 into dotnet:main Nov 18, 2021
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Nov 18, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Nov 18, 2021
@carlossanlop
Copy link
Member

@tmds @adamsitnik just for completeness, can we get perf numbers before and after this change?

@tmds
Copy link
Member Author

tmds commented Nov 19, 2021

can we get perf numbers before and after this change?

Is there some place we can see the diff on the continuous benchmarking of the dotnet/performance repo?

@tmds
Copy link
Member Author

tmds commented Nov 19, 2021

The change wasn't run by the benchmarks yet. Let's see if we can see it in a couple of days.

@adamsitnik what generates this website? Is it part of BenchmarkDotNet?

@adamsitnik
Copy link
Member

The change wasn't run by the benchmarks yet

It's strange, our automation is supposed to run at least few times a day.

what generates this website? Is it part of BenchmarkDotNet?

it's closed source tool maintained by @DrewScoggins

@DrewScoggins
Copy link
Member

Long story short the data on the test history index page only updates about once a day, because it takes about a day to generate. I am looking to make some changes to greatly reduce the time it takes to generate them, but for now that is where we are. As a result they can sometimes be a little behind.

I went ahead and generated a report for just the Directory tests, and though one is marked as a regression, it looks like just noise in the test. I don't see any significant regression or improvement as the result of this change. Report

@tmds
Copy link
Member Author

tmds commented Nov 22, 2021

By now, the benchmark ran a few times with the changes.
newplot

@danmoseley
Copy link
Member

do we need an up for grabs issue to do the same for Windows?

@tmds
Copy link
Member Author

tmds commented Nov 23, 2021

do we need an up for grabs issue to do the same for Windows?

I've created #61954.

@danmoseley
Copy link
Member

thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants