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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Jan 30, 2024
Merged

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

merged 14 commits into from
Jan 30, 2024

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
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
frederick-vs-ja
frederick-vs-ja reviewed Oct 30, 2023
View reviewed changes
stl/inc/filesystem Outdated Show resolved Hide resolved
@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 requested changes Nov 1, 2023
View reviewed changes
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Show resolved Hide resolved
stl/inc/filesystem Show resolved Hide resolved
@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 <[email protected]>
@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
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Outdated Show resolved Hide resolved
stl/inc/filesystem Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jan 25, 2024
@StephanTLavavej
Copy link
Member

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

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
35 checks passed
@StephanTLavavej
Copy link
Member

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

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

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter CaseyCarter approved these changes

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