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

GH-44237: [C#] Use stack allocated buffer when serializing decimal values #44238

Merged

Conversation

georgevanburgh
Copy link
Contributor

@georgevanburgh georgevanburgh commented Sep 26, 2024

Span overrides for decimal.GetBits were added in .NET 5, and allow us to avoid a heap allocation for each decimal value serialized.

Running a quick benchmark shows the expected reduction in allocations

using BenchmarkDotNet.Attributes;

namespace Apache.Arrow.Benchmarks;

[MemoryDiagnoser]
public class DecimalUtilityBenchmark
{
    public decimal Value => 1.00000000m;

    private byte[] _buffer = new byte[16];

    [Benchmark(Baseline = true)]
    public void Baseline() => DecimalUtilityMain.GetBytes(Value, 34, 10, 16, _buffer);

    [Benchmark]
    public void Candidate() => DecimalUtility.GetBytes(Value, 34, 10, 16, _buffer);
}
BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.4894/22H2/2022Update)
13th Gen Intel Core i7-13800H, 1 CPU, 20 logical and 14 physical cores
.NET SDK 8.0.304
  [Host]     : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2
Method Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Baseline 162.7 ns 3.25 ns 4.23 ns 1.00 0.04 0.0088 112 B 1.00
Candidate 152.8 ns 1.38 ns 1.22 ns 0.94 0.02 0.0057 72 B 0.64

Happy to check in some benchmarks for DecimalUtility if they might be useful in future, but thought I'd leave them out for now.

If merged, will close #44237.

Span overrides for decimal.GetBits were added in .NET 5, and allow us to
avoid a heap allocation for each decimal value serialized.
Copy link

⚠️ GitHub issue #44237 has been automatically assigned in GitHub to PR creator.

Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement!

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 26, 2024
@CurtHagenlocher CurtHagenlocher merged commit dcf0d8c into apache:main Sep 26, 2024
10 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting committer review Awaiting committer review label Sep 26, 2024
@georgevanburgh georgevanburgh deleted the decimal-getbits-overload branch September 26, 2024 12:36
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit dcf0d8c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Successfully merging this pull request may close these issues.

[C#] Use new decimal.GetBits overloads to reduce allocations when serializing decimal values
2 participants