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

Improve AppendLiteral for short literals #57217

Merged
merged 3 commits into from
Aug 14, 2021

Conversation

stephentoub
Copy link
Member

DefaultInterpolatedStringHandler.AppendLiteral is special, in that the 99.9% use case is it's called as part of string interpolation by compiler-generated code where the argument is always a literal. That means when the method is inlined, the JIT can specialize the implementation based on knowing the literal's Length is a constant. Which means we can provide specialized implementations for common lengths, resulting in faster and smaller code. This does so for strings of length 1 and 2, as they're quite common in code bases examined, and it does so for both the default handler and also the handler used to format into a span. Just as one data point, a quick-and-dirty search through dotnet/aspnetcore suggests that ~15% of literals are 1-character long.

The exact same code as before gets generated by the JIT for literals of length > 2 (well, at least for the default handler; for the TryWrite one, there's a few more instructions getting generated now for reasons I'm unsure of). The main downside here is if someone does manually call this CompilerServices method with a variable, the call site will end up bloated with the special cases.

Method Toolchain Mean Error StdDev Ratio Allocated Code Size
DefaultOne \main\corerun.exe 66.08 ns 0.983 ns 0.920 ns 1.00 40 B 513 B
DefaultOne \pr\corerun.exe 61.63 ns 0.177 ns 0.157 ns 0.93 40 B 378 B
DefaultTwo \main\corerun.exe 63.90 ns 0.266 ns 0.249 ns 1.00 48 B 516 B
DefaultTwo \pr\corerun.exe 55.60 ns 0.082 ns 0.073 ns 0.87 48 B 390 B
DefaultThree \main\corerun.exe 63.72 ns 0.354 ns 0.314 ns 1.00 48 B 516 B
DefaultThree \pr\corerun.exe 62.17 ns 0.616 ns 0.546 ns 0.98 48 B 516 B
TryWriteOne \main\corerun.exe 35.81 ns 0.057 ns 0.054 ns 1.00 - 506 B
TryWriteOne \pr\corerun.exe 28.60 ns 0.053 ns 0.047 ns 0.80 - 371 B
TryWriteTwo \main\corerun.exe 34.94 ns 0.121 ns 0.101 ns 1.00 - 509 B
TryWriteTwo \pr\corerun.exe 29.30 ns 0.011 ns 0.009 ns 0.84 - 429 B
TryWriteThree \main\corerun.exe 35.19 ns 0.187 ns 0.175 ns 1.00 - 509 B
TryWriteThree \pr\corerun.exe 35.09 ns 0.380 ns 0.337 ns 1.00 - 545 B
private char[] _buffer = new char[100];

[Benchmark]
[Arguments(1, 2, 3, 4)]
public string DefaultOne(uint a, uint b, uint c, uint d) => $"{a}.{b}.{c}.{d}";

[Benchmark]
[Arguments(1, 2, 3, 4)]
public string DefaultTwo(uint a, uint b, uint c, uint d) => $"{a}, {b}, {c}, {d}";

[Benchmark]
[Arguments(1, 2, 3, 4)]
public string DefaultThree(uint a, uint b, uint c, uint d) => $"{a} : {b} : {c} : {d}";

[Benchmark]
[Arguments(1, 2, 3, 4)]
public bool TryWriteOne(uint a, uint b, uint c, uint d) => _buffer.AsSpan().TryWrite($"{a}.{b}.{c}.{d}", out _);

[Benchmark]
[Arguments(1, 2, 3, 4)]
public bool TryWriteTwo(uint a, uint b, uint c, uint d) => _buffer.AsSpan().TryWrite($"{a}, {b}, {c}, {d}", out _);

[Benchmark]
[Arguments(1, 2, 3, 4)]
public bool TryWriteThree(uint a, uint b, uint c, uint d) => _buffer.AsSpan().TryWrite($"{a} : {b} : {c} : {d}", out _);

cc: @GrabYourPitchforks, @EgorBo

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@GrabYourPitchforks
Copy link
Member

Seems like a good use case for #11484, which would allow us to say "pssst, JIT, I'd like this method to go down a different code path based on whether the caller is passing a const into this function." Then you get the best of both worlds: branch elimination for consts, and not including the switch at all for non-consts.

/cc @AndyAyersMS

@GrabYourPitchforks
Copy link
Member

The pattern if ((uint)pos < destination.Length - 1) will widen both sides to 64 bits, which harms codegen, especially in 32-bit processes. But I agree it's correct. ;)

@stephentoub
Copy link
Member Author

The pattern if ((uint)pos < destination.Length - 1) will widen both sides to 64 bits

Yes... is there a better option?

@GrabYourPitchforks
Copy link
Member

Yes... is there a better option?

Not that I'm aware of, if we're trying to be defensive against the possibility of pos being negative or int.MaxValue.

@stephentoub stephentoub force-pushed the appendliteralspecialcase branch from df261dd to 7e95f99 Compare August 11, 2021 18:56
@stephentoub stephentoub force-pushed the appendliteralspecialcase branch from 7e95f99 to 531e3bc Compare August 12, 2021 10:38
@stephentoub
Copy link
Member Author

@SamMonoRT, this is failing a specific CodeDom test consistently with the mono interpreter. Is there someone who could help take a look?

@SamMonoRT
Copy link
Member

@BrzVlad - can you help with the failing interpreter tests ?

DefaultInterpolatedStringHandler.AppendLiteral is special, in that the 99.9% use case is it's called as part of string interpolation by compiler-generated code where the argument is always a literal.  That means when the method is inlined, the JIT can specialize the implementation based on knowing the literal's Length is a constant.  Which means we can provide specialized implementations for common lengths, resulting in faster and smaller code.  This does so for strings of length 1 and 2, as they're quite common in code bases examined, and it does so for both the default handler and also the handler used to format into a span.

The exact same code as before gets generated by the JIT for literals of length > 2.  The main downside here is if someone does manually call this CompilerServices method with a variable, the call site will end up bloated with the special cases.
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants