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

Optimized string concatenation in _Tzdb_update_version() #4577

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

Mq-b
Copy link
Contributor

@Mq-b Mq-b commented Apr 11, 2024

No description provided.

@Mq-b Mq-b requested a review from a team as a code owner April 11, 2024 10:54
frederick-vs-ja

This comment was marked as resolved.

@Mq-b
Copy link
Contributor Author

Mq-b commented Apr 11, 2024

std::string s;
s.resize(33); // Avoid SSO
s = "😅";
std::cout << (const void*)s.data() << '\n';
auto s2 = std::move(s) + "🤣";
std::cout << (const void*)s2.data() << '\n';

std::move is not meaningless and allows for memory reuse.

However, changing operator+ to operato+= is fine by me.

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Apr 11, 2024

not calculating the length of "."

Could ask for this explicitly with "."sv

Note that the result object is now copied from the concatenated string, because operator+= returns an lvalue, so the current change is possibly a pessimization.

Could fix this, and ask for NRVO by separating return statement:

(_Icu_version += '.') += _STD to_string(_Num_leap_seconds);
return _Icu_version;

But I doubt the value of optimizations here.

@Mq-b
Copy link
Contributor Author

Mq-b commented Apr 11, 2024

(_Icu_version += '.') += _STD to_string(_Num_leap_seconds);
return _Icu_version;

this is right.

stl/inc/chrono Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Apr 11, 2024
stl/inc/chrono Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej changed the title Remove meaningless std::move.Optimized string concatenation. Optimized string concatenation in _Tzdb_update_version() Apr 11, 2024
@StephanTLavavej
Copy link
Member

Thanks! I pushed a commit to perform the concatenations on separate lines, since we conventionally don't perform multiple compound assignments on a single line.

Overall, I think this change is reasonable - taking advantage of the NRVO, and using the single-character overload instead of the one for arbitrary C strings. This code is not actually exercised in normal usage (it's bypassed unless the registry reports new leap seconds that we don't know about), so I don't think this is worth extreme micro-optimization, but I'm okay with the change here.

I manually tested this codepath since normal execution doesn't exercise it, and debugged into it to verify that the new code was getting picked up (i.e. verifying that I hadn't forgotten to set_environment.bat):

C:\GitHub\STL\out\x64>type meow.cpp
#include <chrono>
#include <print>
using namespace std;
using namespace std::chrono;

int main() {
    const tzdb& db = get_tzdb();
    println("Original version: {}", db.version);
    println(" Updated version: {}", _Tzdb_update_version(db.version, 1729));
}
C:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /std:c++latest /MTd /Od /Zi /Fdmeow.pdb meow.cpp && meow
meow.cpp
Original version: 2021a.27
 Updated version: 2021a.1729

We merge changes to our GitHub and MSVC-internal repos simultaneously, using a semi-manual "mirroring" process. To save time, we batch up PRs. Your PR will be part of the next batch! I'll post comments here as I prepare that merge.

@StephanTLavavej StephanTLavavej self-assigned this Apr 12, 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 213df0c into microsoft:main Apr 12, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for making the codebase more consistent about using the Named Return Value Optimization, and congratulations on your first microsoft/STL commit! 🎉 😻 🚀

This change is expected to ship in VS 2022 17.11 Preview 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants