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

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

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

elinor-fung
Copy link
Member

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
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: -

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 👍

@elinor-fung elinor-fung merged commit 266bd00 into dotnet:main Aug 17, 2023
169 of 172 checks passed
@elinor-fung elinor-fung deleted the fx-ver-as-str branch August 17, 2023 04:04
@elinor-fung
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants