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

Increase size of Number's small numbers cache, and make it lazy #79061

Merged
merged 4 commits into from
Dec 2, 2022

Conversation

stephentoub
Copy link
Member

Fixes #76888

[MemoryDiagnoser]
public partial class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

    [Benchmark]
    [Arguments(-1)]
    [Arguments(2)]
    [Arguments(20)]
    [Arguments(200)]
    [Arguments(2000)]
    public string Int32ToString(int i) => i.ToString();

    [Benchmark]
    [Arguments(-1)]
    [Arguments(2)]
    [Arguments(20)]
    [Arguments(200)]
    [Arguments(2000)]
    public string Int64ToString(long i) => i.ToString();
}
Method Toolchain i Mean Error StdDev Ratio Allocated
Int32ToString \main\corerun.exe 2 1.5308 ns 0.0915 ns 0.0856 ns 1.00 -
Int32ToString \pr\corerun.exe 2 3.0073 ns 0.1173 ns 0.1826 ns 1.97 -
Int64ToString \main\corerun.exe 2 2.1372 ns 0.0856 ns 0.0759 ns 1.00 -
Int64ToString \pr\corerun.exe 2 3.5599 ns 0.1288 ns 0.2042 ns 1.73 -
Int32ToString \main\corerun.exe 20 9.6048 ns 0.2319 ns 0.2170 ns 1.00 32 B
Int32ToString \pr\corerun.exe 20 2.7729 ns 0.1113 ns 0.1325 ns 0.29 -
Int64ToString \main\corerun.exe 20 10.6359 ns 0.2711 ns 0.4058 ns 1.00 32 B
Int64ToString \pr\corerun.exe 20 3.3825 ns 0.1306 ns 0.2071 ns 0.32 -
Int32ToString \main\corerun.exe 200 10.1559 ns 0.2582 ns 0.3619 ns 1.00 32 B
Int32ToString \pr\corerun.exe 200 2.4571 ns 0.0307 ns 0.0256 ns 0.24 -
Int64ToString \main\corerun.exe 200 11.5714 ns 0.2896 ns 0.5072 ns 1.00 32 B
Int64ToString \pr\corerun.exe 200 3.5913 ns 0.1289 ns 0.2691 ns 0.31 -
Int32ToString \main\corerun.exe 2000 10.4373 ns 0.2693 ns 0.3307 ns 1.00 32 B
Int32ToString \pr\corerun.exe 2000 10.4839 ns 0.2502 ns 0.3588 ns 1.00 32 B
Int64ToString \main\corerun.exe 2000 11.8617 ns 0.2975 ns 0.5051 ns 1.00 32 B
Int64ToString \pr\corerun.exe 2000 12.2352 ns 0.2989 ns 0.3198 ns 1.03 32 B
Int32ToString \main\corerun.exe -1 18.9890 ns 0.4320 ns 0.9111 ns 1.00 32 B
Int32ToString \pr\corerun.exe -1 19.8757 ns 0.4461 ns 0.6254 ns 1.05 32 B
Int64ToString \main\corerun.exe -1 18.2829 ns 0.4263 ns 0.9796 ns 1.00 32 B
Int64ToString \pr\corerun.exe -1 18.4973 ns 0.4308 ns 0.6448 ns 1.02 32 B

@stephentoub stephentoub requested a review from jkotas November 30, 2022 23:05
@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.

@ghost ghost assigned stephentoub Nov 30, 2022
@jkotas
Copy link
Member

jkotas commented Dec 1, 2022

I assume that the perf regression for i: 2 is some micro-architecture outlier. I do not see anything in the code that would explain it.

@stephentoub
Copy link
Member Author

I assume that the perf regression for i: 2 is some micro-architecture outlier. I do not see anything in the code that would explain it.

Maybe? I'd assumed it was due to switching to the strings being lazily initialized, so there's now a null check / branch where there wasn't one for single digits.

@jkotas
Copy link
Member

jkotas commented Dec 1, 2022

I'd assumed it was due to switching to the strings being lazily initialized, so there's now a null check / branch where there wasn't one for single digits.

Yes, there is an extra conditional branch, but it should be perfectly predicted in the microbenchmark run. It is not enough to explain close to 2x slowdown.

@stephentoub
Copy link
Member Author

@BrzVlad, can you validate the interpreter changes here? This PR is replacing the cache that intrinsic was using. If there's something I should be doing instead, please let me know.

@stephentoub
Copy link
Member Author

Spoke offline with Vlad about the interpreter, and we're good to remove this hack (plan was to remove it anyway at some point soon).

@stephentoub
Copy link
Member Author

Failure is #79124

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider expanding number formatting cache to larger values
2 participants