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

Optimize shift operations for BigInteger #112632

Closed
wants to merge 5 commits into from
Closed

Conversation

kzrnm
Copy link
Contributor

@kzrnm kzrnm commented Feb 17, 2025

Fix #112564

This pull request includes the following changes:

Benchmark


BenchmarkDotNet v0.13.12, Windows 11 (10.0.26100.3194)
13th Gen Intel Core i5-13500, 1 CPU, 20 logical and 14 physical cores
.NET SDK 10.0.100-alpha.1.25077.2
  [Host]     : .NET 10.0.0 (10.0.25.7313), X64 RyuJIT AVX2
  Job-XLUTTB : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-KGFXGU : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX2


Method Toolchain Mean Ratio Gen0 Gen1 Gen2 Allocated Alloc Ratio
PositiveLeftShift \main\corerun.exe 13.920 ms 1.00 62.5000 62.5000 62.5000 33 MB 1.00
PositiveLeftShift \pr\corerun.exe 9.485 ms 0.68 156.2500 156.2500 156.2500 33 MB 1.00
NegativeLeftShift \main\corerun.exe 13.567 ms 1.00 78.1250 78.1250 78.1250 33 MB 1.00
NegativeLeftShift \pr\corerun.exe 9.134 ms 0.67 156.2500 156.2500 156.2500 33 MB 1.00
PositiveRightShift \main\corerun.exe 16.709 ms 1.00 125.0000 125.0000 125.0000 31 MB 1.00
PositiveRightShift \pr\corerun.exe 12.411 ms 0.74 187.5000 187.5000 187.5000 31 MB 1.00
NegativeRightShift \main\corerun.exe 21.708 ms 1.00 125.0000 125.0000 125.0000 31 MB 1.00
NegativeRightShift \pr\corerun.exe 12.291 ms 0.57 187.5000 187.5000 187.5000 31 MB 1.00
PositiveUnsignedRightShift \main\corerun.exe 15.083 ms 1.00 125.0000 125.0000 125.0000 31 MB 1.00
PositiveUnsignedRightShift \pr\corerun.exe 12.585 ms 0.84 187.5000 187.5000 187.5000 31 MB 1.00
NegativeUnsignedRightShift \main\corerun.exe 18.347 ms 1.00 125.0000 125.0000 125.0000 31 MB 1.00
NegativeUnsignedRightShift \pr\corerun.exe 14.719 ms 0.81 187.5000 187.5000 187.5000 31 MB 1.00
PositiveRotateLeft \main\corerun.exe 15.150 ms 1.00 125.0000 125.0000 125.0000 32 MB 1.00
PositiveRotateLeft \pr\corerun.exe 13.825 ms 0.90 187.5000 187.5000 187.5000 32 MB 1.00
NegativeRotateLeft \main\corerun.exe 19.346 ms 1.00 125.0000 125.0000 125.0000 32 MB 1.00
NegativeRotateLeft \pr\corerun.exe 18.920 ms 0.97 187.5000 187.5000 187.5000 32 MB 1.00
PositiveRotateRight \main\corerun.exe 14.727 ms 1.00 125.0000 125.0000 125.0000 32 MB 1.00
PositiveRotateRight \pr\corerun.exe 14.222 ms 0.97 187.5000 187.5000 187.5000 32 MB 1.00
NegativeRotateRight \main\corerun.exe 16.936 ms 1.00 125.0000 125.0000 125.0000 32 MB 1.00
NegativeRotateRight \pr\corerun.exe 16.650 ms 0.98 187.5000 187.5000 187.5000 32 MB 1.00
benchmark code
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using System.Numerics;

[MemoryDiagnoser(false)]
[HideColumns("Job", "Error", "StdDev", "Median", "RatioSD")]
public class ShiftTest
{
    const int shiftSize = (1 << 23) + 13;
    BigInteger positive, negative;

    [GlobalSetup]
    public void Setup()
    {
        var bytes = new byte[1 << 25];
        new Random(227).NextBytes(bytes);
        positive = new BigInteger(bytes, isUnsigned: true);
        negative = -positive;
    }

    [Benchmark] public BigInteger PositiveLeftShift() => positive << shiftSize;
    [Benchmark] public BigInteger NegativeLeftShift() => negative << shiftSize;

    [Benchmark] public BigInteger PositiveRightShift() => positive >> shiftSize;
    [Benchmark] public BigInteger NegativeRightShift() => negative >> shiftSize;

    [Benchmark] public BigInteger PositiveUnsignedRightShift() => positive >>> shiftSize;
    [Benchmark] public BigInteger NegativeUnsignedRightShift() => negative >>> shiftSize;

    [Benchmark] public BigInteger PositiveRotateLeft() => BigInteger.RotateLeft(positive, shiftSize);
    [Benchmark] public BigInteger NegativeRotateLeft() => BigInteger.RotateLeft(negative, shiftSize);

    [Benchmark] public BigInteger PositiveRotateRight() => BigInteger.RotateRight(positive, shiftSize);
    [Benchmark] public BigInteger NegativeRotateRight() => BigInteger.RotateRight(negative, shiftSize);
}

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 17, 2025

// Do an in-place one's complement. "Dangerous" because it causes
// a mutation and needs to be used with care for immutable types.
public static void DangerousMakeOnesComplement(Span<uint> d)
Copy link
Member

Choose a reason for hiding this comment

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

If it accepts a non-readonly span - what is dangerous with it? We typically name such things with "Inplace"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the existing DangerousMakeTwosComplement. I have no strong preference for the naming convention, but if DangerousMakeOnesComplement is to be renamed, then DangerousMakeTwosComplement should be renamed as well.

public static void DangerousMakeTwosComplement(Span<uint> d)

Copy link
Member

Choose a reason for hiding this comment

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

np. It just that it's the code that converts an immutable data into mutable span is what actually dangerous 🙂

@huoyaoyuan
Copy link
Member

Fixed shifting for negative numbers

This is indeed behavioral change, could require design decision and breaking change announcement.

@kzrnm
Copy link
Contributor Author

kzrnm commented Feb 24, 2025

I noticed other compatibility-related problem, so I will esubmit new PR.

@kzrnm kzrnm closed this Feb 24, 2025
@huoyaoyuan
Copy link
Member

I noticed other compatibility-related problem, so I will esubmit new PR.

I'd suggest to confirm the proposed behavioral change in the issue first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The unsigned right shift operator (>>>) of BigInteger behaves strangely
3 participants