dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.3k stars 4.74k forks source link

BigInteger.MaxMagnitude #105324

Open PetSerAl opened 3 months ago

PetSerAl commented 3 months ago

https://github.com/dotnet/runtime/blob/53d1a6d8afa19e3c0cebdad3004325edf954d3d9/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs#L4095-L4114

Since BigInteger comparison could be costly, should not two comparisons (ax > ay and ax == ay) be replaced with single CompareTo call and switching on result?

return
  Abs(x).CompareTo(Abs(y)) switch {
    > 0 => x,
    0 => IsNegative(x) ? y : x,
    _ => y
  };
dotnet-policy-service[bot] commented 3 months ago

Tagging subscribers to this area: @dotnet/area-system-numerics See info in area-owners.md if you want to be subscribed.

huoyaoyuan commented 3 months ago

Creating Abs value for BigInteger tends to be much more costly as it allocates. Comparing BigIntegers is not so expensive for non-huge numbers, as they are vectorizable.

PaulusParssinen commented 3 months ago

Creating Abs value for BigInteger tends to be much more costly as it allocates.

The existing code already is taking absolute values of both operands. This should still be slight overall improvement, right?

PetSerAl commented 3 months ago

Creating Abs value for BigInteger tends to be much more costly as it allocates.

Abs does not allocate.

https://github.com/dotnet/runtime/blob/53d1a6d8afa19e3c0cebdad3004325edf954d3d9/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs#L709-L712

https://github.com/dotnet/runtime/blob/53d1a6d8afa19e3c0cebdad3004325edf954d3d9/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs#L2644-L2648

huoyaoyuan commented 3 months ago

Abs does not allocate.

Oh I forgot bits stores absolute value instead of raw value...never mind.

Yes, doing comparison only once does save.

PetSerAl commented 3 months ago

It seems Abs could also be improved with:

return new BigInteger(Math.Abs(value._sign), value._bits);
karakasa commented 3 months ago

We should also optimize Abs() though. Let me work on a PR later.

Method Length Mean Error StdDev Ratio Allocated Alloc Ratio
Original_WorstPath 16 14.150 ns 0.0308 ns 0.0289 ns 1.00 - NA
CompareTo_WorstPath 16 11.860 ns 0.0318 ns 0.0297 ns 0.84 - NA
CompareToAndAbs_WorstPath 16 7.630 ns 0.0245 ns 0.0204 ns 0.54 - NA
Original_WorstPath 256 82.871 ns 0.0887 ns 0.0740 ns 1.00 - NA
CompareTo_WorstPath 256 49.875 ns 0.0941 ns 0.0834 ns 0.60 - NA
CompareToAndAbs_WorstPath 256 36.596 ns 0.1276 ns 0.1066 ns 0.44 - NA
Original_WorstPath 4096 990.591 ns 2.6575 ns 2.4858 ns 1.00 - NA
CompareTo_WorstPath 4096 499.955 ns 1.6945 ns 1.5850 ns 0.50 - NA
CompareToAndAbs_WorstPath 4096 495.232 ns 1.2101 ns 1.1320 ns 0.50 - NA
Original_WorstPath 65536 15,411.929 ns 41.0962 ns 38.4414 ns 1.00 - NA
CompareTo_WorstPath 65536 7,711.964 ns 24.9932 ns 23.3786 ns 0.50 - NA
CompareToAndAbs_WorstPath 65536 7,702.426 ns 24.3057 ns 22.7355 ns 0.50 - NA
code ```C# using BenchmarkDotNet.Attributes; using System.Numerics; namespace benchmark; [MemoryDiagnoser] [Config(typeof(AntiVirusFriendlyConfig))] public class BigNumberTest { [Params(16, 256, 4096, 65536)] public int Length { get; set; } public BigInteger A; public BigInteger B; [GlobalSetup] public void PrepareBigNumber() { var rng = new Random(42); var array = new byte[Length]; rng.NextBytes(array); A = new BigInteger(array); B = -new BigInteger(array); } [Benchmark(Baseline = true)] public BigInteger Original_WorstPath() { return BigInteger.MaxMagnitude(A, B); } [Benchmark] public BigInteger CompareTo_WorstPath() { return MaxMag1(A, B); } [Benchmark] public BigInteger CompareToAndAbs_WorstPath() { return MaxMag2(A, B); } private static BigInteger MaxMag1(BigInteger x, BigInteger y) { return BigInteger.Abs(x).CompareTo(BigInteger.Abs(y)) switch { > 0 => x, 0 => BigInteger.IsNegative(x) ? y : x, _ => y }; } private static BigInteger Abs(BigInteger bi) { return BigInteger.IsNegative(bi) ? -bi : bi; } private static BigInteger MaxMag2(BigInteger x, BigInteger y) { return Abs(x).CompareTo(Abs(y)) switch { > 0 => x, 0 => BigInteger.IsNegative(x) ? y : x, _ => y }; } } ```
PetSerAl commented 3 months ago

@karakasa WorstPath? In your code A and B share the same array. That means comparison can be done by comparing array reference. No need to compare array content.

PetSerAl commented 3 months ago

@karakasa But == have take advantage of that.

karakasa commented 3 months ago

@karakasa But == have take advantage of that.

You are right. 👍 Thank you for the correction. Benchmark has been updated.