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

Minimize System.Version formatting buffer's length #102508

Closed
wants to merge 3 commits into from
Closed

Minimize System.Version formatting buffer's length #102508

wants to merge 3 commits into from

Conversation

Chasmical
Copy link

Change the length of the char buffer used in Version.ToString() from 47 to 43 chars (4 non-negative Int32s + 3 periods). 2147483647.2147483647.2147483647.2147483647. Even if a version has invalid internal state (any component less than -1), the TryFormat method just casts them to UInt32 anyway.

Change the length of the char buffer used in Version.ToString()
from 47 to 43 chars (4 non-negative Int32s + 3 periods). Even if
a version has invalid internal state (any component less than -1),
the TryFormat method just casts them to UInt32 anyway.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 21, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 21, 2024
@Chasmical
Copy link
Author

Oh, I didn't realize that Number.UInt32NumberBufferLength is the same as Int32NumberBufferLength - 11, for some reason.

@EgorBo
Copy link
Member

EgorBo commented May 21, 2024

To be fair, this doesn't reduce the actual stack allocation size - JIT is going to round up both to % 16 anyway, e.g. see sharplab

Number.Int32NumberBufferLength is only used in System.Number class and
here. The extra 1 to length is designated for the terminating nulls.
It would make more sense not to use it here.
@Chasmical
Copy link
Author

I see. 😄 Then, I guess it could still improve readability of the decompiled code. It was confusing to see 47 characters allocated when the actual maximum length is 43.

On another note, looks like Number.Int32NumberBufferLength isn't used anywhere else, besides the Number class and here, and the extra 1 to length is actually for the terminating nulls. Would make more sense not to use it here, in case Number.NumberBuffer changes.

// We need 1 additional byte, per length, for the terminating null
internal const int DecimalNumberBufferLength = 29 + 1 + 1; // 29 for the longest input + 1 for rounding
internal const int DoubleNumberBufferLength = 767 + 1 + 1; // 767 for the longest input + 1 for rounding: 4.9406564584124654E-324
internal const int Int32NumberBufferLength = 10 + 1; // 10 for the longest input: 2,147,483,647

@stephentoub
Copy link
Member

Thank you, but I'd rather keep it how it is today. I think it's more maintainable as it is currently and this doesn't actually improve perf.

@Chasmical Chasmical closed this May 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants