Skip to content

Switch fx_ver_t::as_str from stringstream to append/to_string #90652

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
Merged
elinor-fung merged 3 commits into from
Aug 17, 2023
Merged

Switch fx_ver_t::as_str from stringstream to append/to_string #90652

elinor-fung merged 3 commits into from
Aug 17, 2023

Conversation

@elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Aug 16, 2023

Stop using std::stringstream to convert fx_ver_t to a string. It pulls in std::locale (and other things) in all the host binaries when we really just need a simple conversion of the numbers to a string. In some binaries, it was also the only usage of stringstream. In theory, this should also be faster, although this is not a particularly perf-sensitive function (most usage is for when tracing is enabled).

On my local Windows x64 build, the sizes on disk were:

before (kB) after (kB) diff (kB)
apphost 153 136 -17
comhost 189 174 -15
dotnet 172 129 -43
ijwhost 141 101 -40
nethost 131 88 - 43

@ghost ghost assigned elinor-fung Aug 16, 2023
@ghost
Copy link

ghost commented Aug 16, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Stop using std::stringstream to convert fx_ver_t to a string. It pulls in std::locale (and other things) in all the host binaries when we really just need a simple conversion of the numbers to a string. In some binaries, it was also the only usage of stringstream. In theory, this should also be faster, although this is not a particularly perf-sensitive function (most usage is for when tracing is enabled).

On my local Windows x64 build, the sizes on disk were:

before (kB) after (kB) diff (kB)
apphost 153 136 -17
comhost 189 174 -15
dotnet 172 129 -43
ijwhost 141 101 -40
nethost 131 88 - 43
Author: elinor-fung
Assignees: -
Labels:

area-Host
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Aug 16, 2023
Closed
am11
am11 reviewed Aug 16, 2023
View reviewed changes
vitek-karas
vitek-karas approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Nice 👍

This was referenced Aug 17, 2023
Closed
Open
@elinor-fung elinor-fung merged commit 266bd00 into dotnet:main Aug 17, 2023
@elinor-fung elinor-fung deleted the fx-ver-as-str branch August 17, 2023 04:04
@elinor-fung
Copy link
Member Author

elinor-fung commented Aug 17, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2023

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5894030092

@github-actions github-actions bot mentioned this pull request Aug 17, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@vitek-karas vitek-karas vitek-karas approved these changes

+1 more reviewer

@am11 am11 am11 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@elinor-fung elinor-fung

Labels

area-Host

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@elinor-fung @jkoritzinsky @am11 @vitek-karas