Skip to content

Reduce allocations for CreateDirectory - #61777

Merged
adamsitnik merged 7 commits into
dotnet:mainfrom
adamsitnik:removeAllocs
Nov 19, 2021
Merged

Reduce allocations for CreateDirectory#61777
adamsitnik merged 7 commits into
dotnet:mainfrom
adamsitnik:removeAllocs

Conversation

@adamsitnik

@adamsitnikadamsitnik commented Nov 18, 2021•
edited
Loading

Copy link
Copy Markdown
Member

I wanted to see what would it take to have a sys-call that accepts a ROS<char> instead of string and after that I've discovered the existence of ValueListBuilder<int> which allowed me to get rid of List<int> allocation.

BenchmarkDotNet=v0.13.1.1616-nightly, OS=ubuntu 18.04
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.100-alpha.1.21566.20
MethodToolchaindepthMeanRatioAllocated
RecursiveCreateDeleteDirectory/before/corerun10314.9 us1.008 KB
RecursiveCreateDeleteDirectory/after/corerun10296.1 us0.957 KB
RecursiveCreateDeleteDirectory/before/corerun1003,565.2 us1.00140 KB
RecursiveCreateDeleteDirectory/after/corerun1003,559.1 us1.00113 KB

@adamsitnikadamsitnik added area-System.IO tenet-performance Performance related issue labels Nov 18, 2021
@adamsitnikadamsitnik added this to the 7.0.0 milestone Nov 18, 2021
@adamsitnik
adamsitnik requested a review from tmdsNovember 18, 2021 14:52
@ghost

ghost commented Nov 18, 2021

Copy link
Copy Markdown

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

Issue Details

I wanted to see what would it take to have a sys-call that accepts a ROS<char> instead of string and after that I've discovered the existence of ValueListBuilder<int> which allowed me to get rid of List<int> allocation.

Author:adamsitnik
Assignees:-
Labels:

area-System.IO, tenet-performance

Milestone:7.0.0

@adamsitnikadamsitnik added the os-linux Linux OS (any supported distro) label Nov 18, 2021
stephentoub
stephentoub reviewed Nov 18, 2021
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.MkDir.cs
stephentoub
stephentoub reviewed Nov 18, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated
stephentoub
stephentoub reviewed Nov 18, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated
stephentoub
stephentoub approved these changes Nov 18, 2021
View reviewed changes

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Other than passing the VLB by ref (which needs to be done), LGTM.

@adamsitnik@stephentoub
Apply suggestions from code review
0183954
Co-authored-by: Stephen Toub <stoub@microsoft.com>
carlossanlop
carlossanlop reviewed Nov 18, 2021
View reviewed changes

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI @tmds has a PR open that's touching directory deletion APIs.

Maybe the deletion APIs could benefit from a similar allocation reduction?

#59520

Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.MkDir.cs
carlossanlop
carlossanlop approved these changes Nov 18, 2021
View reviewed changes

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, I meant to approve it, not comment.

LGTM (pending hearing from @AaronRobinsonMSFT in Stephen's question).

tmds
tmds reviewed Nov 18, 2021
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated
tmds
tmds reviewed Nov 18, 2021
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Unix/System.Native/Interop.MkDir.cs Outdated
@adamsitnik
adamsitnik requested a review from tmdsNovember 19, 2021 07:54
@tmds

tmds commented Nov 19, 2021

Copy link
Copy Markdown
Member

I learned a few new tricks from this PR. Thanks @adamsitnik!

@adamsitnik
adamsitnik merged commit ba4eae0 into dotnet:mainNov 19, 2021
@adamsitnik
adamsitnik deleted the removeAllocs branch November 19, 2021 12:02
@tmdstmds mentioned this pull request Nov 23, 2021
Closed
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AaronRobinsonMSFTAaronRobinsonMSFTAaronRobinsonMSFT left review comments
@stephentoubstephentoubstephentoub approved these changes
@jozkeejozkeeAwaiting requested review from jozkee
+2 more reviewers
@tmdstmdstmds approved these changes
@carlossanlopcarlossanlopcarlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.IOos-linuxLinux OS (any supported distro)tenet-performancePerformance related issue

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@adamsitnik@tmds@carlossanlop@stephentoub@AaronRobinsonMSFT