Skip to content

<filesystem>: Preallocate memory in path::operator/#4136

Merged
StephanTLavavej merged 14 commits intomicrosoft:mainfrom
xenu:xenu/path-concat
Jan 30, 2024
Merged

<filesystem>: Preallocate memory in path::operator/#4136
StephanTLavavej merged 14 commits intomicrosoft:mainfrom
xenu:xenu/path-concat

Conversation

@xenu
Copy link
Contributor

@xenu xenu commented Oct 30, 2023 •
edited
Loading

Now, in the typical case, it will do at most a single allocation.

It's a hot path in my program, and a cursory GitHub search showed that path::operator/ is indeed commonly used.

@xenu
<filesystem>: Preallocate memory in path::operator/
f968273 
Now, in the typical case, it will do at most a single allocation.

It's a hot path in my program, and a cursory GitHub search showed
that path::operator/ is indeed commonly used.
@xenu xenu requested a review from a team as a code owner October 30, 2023 04:20
@xenu
Copy link
Contributor Author

xenu commented Oct 30, 2023

@microsoft-github-policy-service agree

AlexGuteniev
AlexGuteniev reviewed Oct 30, 2023
View reviewed changes
frederick-vs-ja
frederick-vs-ja reviewed Oct 30, 2023
View reviewed changes
@StephanTLavavej StephanTLavavej added performance Must go faster filesystem C++17 filesystem labels Oct 30, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 31, 2023
CaseyCarter
CaseyCarter suggested changes Nov 1, 2023
View reviewed changes
@StephanTLavavej StephanTLavavej removed their assignment Nov 1, 2023
@StephanTLavavej StephanTLavavej self-assigned this Nov 15, 2023
StephanTLavavej and others added 11 commits January 25, 2024 06:31
@StephanTLavavej
Merge branch 'main' into xenu/path-concat
ee16754
@StephanTLavavej
Fuse comments to avoid looking like commented-out code.
4b089ca
@StephanTLavavej
Reuse _Left_first, consistent with _Right_MEOW above.
404cd63
@StephanTLavavej
Capture by copy, which might help alias analysis.
f79439a
@StephanTLavavej @CaseyCarter
Check _Right_size != 0 before inspecting *_Right_first.
abacb4f 
Co-authored-by: Casey Carter <cacarter@microsoft.com>
@StephanTLavavej
Grammar: "would've made" => "would make"
1f0a307 
We're talking about something in the future that we want to avoid doing,
instead of something in the past that we avoided doing.
@StephanTLavavej
The _Size parameter can be const.
a488f8d
@StephanTLavavej
Style: static_cast<size_t> before adding bool.
23cf2c2
@StephanTLavavej
Test both operator/= and operator/.
6d384e7
@StephanTLavavej
Test the both-empty case.
f4637f4 
This doesn't exercise the optimization, it's just a good idea.
@StephanTLavavej
Add a test case to exercise the optimization.
381580f 
With this, I believe all codepaths are exercised.
StephanTLavavej
StephanTLavavej reviewed Jan 25, 2024
View reviewed changes
@StephanTLavavej StephanTLavavej removed their assignment Jan 25, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jan 25, 2024

Thank you! 😻 Apologies for the (somewhat holiday-related) delay in getting this reviewed. I've pushed a conflict-free merge with main, followed by changes to address previous feedback, followed by changes for things I noticed, including test coverage. The logic looks solid (the only thing that needed a small patch was _Right being empty, and even that was probably technically well-defined due to basic_string null termination) - I really appreciate how you handled the strange corner cases.

We have a semi-manual process for simultaneously merging PRs to the GitHub and MSVC-internal repos. To save time, we batch up PRs, so your PR will be part of the next batch (possibly tomorrow but more likely next week).

@xenu
Copy link
Contributor Author

xenu commented Jan 26, 2024

Sorry for ignoring the review comments, I got busy with other stuff and then I forgot about this PR.

I've pushed a conflict-free merge with main, followed by changes to address previous feedback, followed by changes for things I noticed, including test coverage.

Thank you!

@StephanTLavavej StephanTLavavej self-assigned this Jan 30, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jan 30, 2024

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 9c10c1a into microsoft:main Jan 30, 2024
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Jan 30, 2024

Thanks for optimizing this important function, and congratulations on your first microsoft/STL commit! 🚀 😻 🎉

This is expected to ship in VS 2022 17.10 Preview 2.

@xenu xenu deleted the xenu/path-concat branch February 14, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

filesystem C++17 filesystem performance Must go faster

Projects

STL Code Reviews
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xenu @StephanTLavavej @CaseyCarter @AlexGuteniev @frederick-vs-ja

Comments