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

Utf8Formatter.TryFormat causes massive code bloat when used with a constant StandardFormat #33349

Closed
Thealexbarney opened this issue Mar 8, 2020 · 4 comments · Fixed by #52708
Labels
Milestone

Comments

@Thealexbarney
Copy link

The following function compiles to ~900 bytes of code when jitted on x64.

private static bool Format(Span<byte> span, int value)
{
    return Utf8Formatter.TryFormat(value, span, out _, new StandardFormat('D', 2));
}

Adding AggressiveInlining to StandardFormat.ctor allows the JIT to reduce the TryFormat call to a single call to TryFormatUInt64D, resulting in a code size of less than 100 bytes.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Mar 8, 2020
@EgorBo
Copy link
Member

EgorBo commented Mar 8, 2020

This change: EgorBo@b7f15b7

makes StandardFormat.ctor inlineable 😄 (removes a branch+bb basically)
(And it seems it does make sense to make it so since it's mostly used with constant arguments)

@jkotas
Copy link
Member

jkotas commented Mar 8, 2020

TryFormatUInt64D, resulting in a code size of less than 100 bytes

That is still very questionable. Some of the AggressiveInlining in the Utf8Formatter code should be deleted.

@jkotas jkotas added this to the 5.0 milestone Mar 8, 2020
@jkotas jkotas added the tenet-performance Performance related issue label Mar 8, 2020
@GrabYourPitchforks
Copy link
Member

I agree that some of the AggressiveInlining attributes should disappear. The tricks I was playing around with over at #32843 and Andy's WIP PR should also be generally applicable here, which could further reduce the codegen size around the switch statement.

@Thealexbarney
Copy link
Author

Ideally all the dispatch code in TryFormatUInt64/TryFormatInt64 would disappear if the format was a constant. AggressiveInlining can't differentiate between the case of using a constant or not, so the entire TryFormatUInt64 function would always be inlined in non-constant cases.

I'd say that TryFormatUInt64Default should definitely not have AggressiveInlining if TryFormatUInt64 also has AggressiveInlining. TryFormatUInt64Default's child functions should keep AggressiveInlining since they would be inlined twice at most throughout the entire program.

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@danmoseley danmoseley modified the milestones: 5.0.0, Future Aug 12, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 30, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants