Use Span.CopyTo(Span) in BigInteger's add and subtract by speshuric · Pull Request #83951 · dotnet/runtime

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

speshuric · 2023-03-26T21:44:25Z

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

ghost · 2023-03-26T21:44:37Z

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

Issue Details

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result.

With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part.

See #83457 for details.

Do not merge until discussed.

Author:	speshuric
Assignees:	-
Labels:	`area-System.Numerics`, `community-contribution`
Milestone:	-

In BigIntegerCalculator methods Add and Subtract, if sizes of arguments differ, after processing the part of size of the right (small) argument, there was loop of add/sub carry value. When the carry value once become zero, in fact the rest of the larger argument can be copied to the result. With this commit the second loop is interrupted when carry become zero and applies fast Span.CopyTo(Span) to the rest part. This optimization applied only when size of the greatest argument is more or equal to const CopyToThreshold introduced in this commit. This const is 8 now. Also made minor related changes to hot cycles. See #83457 for details.

adamsitnik

I've synced your fork locally with main (to include most recent JIT changes) and compared it against main. The performance gains are impressive, great job @speshuric !

Details

Method	Job	arguments	Mean	Ratio	Allocated	Alloc Ratio
Add	PR	128,128 bits	20.45 ns	0.97	48 B	1.00
Add	main	128,128 bits	21.02 ns	1.00	48 B	1.00

Add	PR	128,2048 bits	57.58 ns	0.68	280 B	1.00
Add	main	128,2048 bits	84.75 ns	1.00	280 B	1.00

Add	PR	128,32 bits	19.67 ns	0.94	40 B	1.00
Add	main	128,32 bits	20.98 ns	1.00	40 B	1.00

Add	PR	128,512 bits	25.22 ns	0.92	88 B	1.00
Add	main	128,512 bits	27.46 ns	1.00	88 B	1.00

Add	PR	128,8192 bits	89.94 ns	0.44	1048 B	1.00
Add	main	128,8192 bits	206.56 ns	1.00	1048 B	1.00

Add	PR	131072,128 bits	827.94 ns	0.30	16408 B	1.00
Add	main	131072,128 bits	2,781.52 ns	1.00	16408 B	1.00

Add	PR	131072,2048 bits	927.65 ns	0.33	16408 B	1.00
Add	main	131072,2048 bits	2,994.30 ns	1.00	16408 B	1.00

Add	PR	131072,32 bits	856.88 ns	0.29	16408 B	1.00
Add	main	131072,32 bits	2,928.15 ns	1.00	16408 B	1.00

Add	PR	131072,512 bits	932.48 ns	0.31	16408 B	1.00
Add	main	131072,512 bits	3,017.33 ns	1.00	16408 B	1.00

Add	PR	131072,8192 bits	1,051.76 ns	0.35	16408 B	1.00
Add	main	131072,8192 bits	2,981.83 ns	1.00	16408 B	1.00

Add	PR	2048,128 bits	50.38 ns	0.61	280 B	1.00
Add	main	2048,128 bits	81.90 ns	1.00	280 B	1.00

Add	PR	2048,2048 bits	88.24 ns	0.99	288 B	1.00
Add	main	2048,2048 bits	89.13 ns	1.00	288 B	1.00

Add	PR	2048,32 bits	52.21 ns	0.65	280 B	1.00
Add	main	2048,32 bits	80.10 ns	1.00	280 B	1.00

Add	PR	2048,512 bits	57.34 ns	0.69	280 B	1.00
Add	main	2048,512 bits	82.59 ns	1.00	280 B	1.00

Add	PR	2048,8192 bits	125.71 ns	0.55	1048 B	1.00
Add	main	2048,8192 bits	229.65 ns	1.00	1048 B	1.00

Add	PR	2097152,128 bits	22,540.72 ns	0.37	262204 B	1.00
Add	main	2097152,128 bits	60,474.17 ns	1.00	262214 B	1.00

Add	PR	2097152,2048 bits	23,170.94 ns	0.41	262203 B	1.00
Add	main	2097152,2048 bits	56,260.18 ns	1.00	262214 B	1.00

Add	PR	2097152,32 bits	27,136.58 ns	0.42	262206 B	1.00
Add	main	2097152,32 bits	64,525.60 ns	1.00	262215 B	1.00

Add	PR	2097152,512 bits	22,966.01 ns	0.40	262203 B	1.00
Add	main	2097152,512 bits	56,140.32 ns	1.00	262214 B	1.00

Add	PR	2097152,8192 bits	23,176.13 ns	0.40	262203 B	1.00
Add	main	2097152,8192 bits	56,357.32 ns	1.00	262214 B	1.00

Add	PR	32,128 bits	19.22 ns	0.95	40 B	1.00
Add	main	32,128 bits	20.18 ns	1.00	40 B	1.00

Add	PR	32,2048 bits	51.55 ns	0.63	280 B	1.00
Add	main	32,2048 bits	81.77 ns	1.00	280 B	1.00

Add	PR	32,32 bits	17.60 ns	0.99	32 B	1.00
Add	main	32,32 bits	17.69 ns	1.00	32 B	1.00

Add	PR	32,512 bits	23.69 ns	0.86	88 B	1.00
Add	main	32,512 bits	27.59 ns	1.00	88 B	1.00

Add	PR	32,8192 bits	86.92 ns	0.39	1048 B	1.00
Add	main	32,8192 bits	222.72 ns	1.00	1048 B	1.00

Add	PR	32768,128 bits	224.63 ns	0.32	4120 B	1.00
Add	main	32768,128 bits	698.71 ns	1.00	4120 B	1.00

Add	PR	32768,2048 bits	272.23 ns	0.38	4120 B	1.00
Add	main	32768,2048 bits	721.08 ns	1.00	4120 B	1.00

Add	PR	32768,32 bits	226.28 ns	0.29	4120 B	1.00
Add	main	32768,32 bits	771.12 ns	1.00	4120 B	1.00

Add	PR	32768,512 bits	231.20 ns	0.33	4120 B	1.00
Add	main	32768,512 bits	704.56 ns	1.00	4120 B	1.00

Add	PR	32768,8192 bits	412.62 ns	0.55	4120 B	1.00
Add	main	32768,8192 bits	754.78 ns	1.00	4120 B	1.00

Add	PR	512,128 bits	24.13 ns	0.87	88 B	1.00
Add	main	512,128 bits	27.55 ns	1.00	88 B	1.00

Add	PR	512,2048 bits	59.59 ns	0.68	280 B	1.00
Add	main	512,2048 bits	87.00 ns	1.00	280 B	1.00

Add	PR	512,32 bits	22.30 ns	0.83	88 B	1.00
Add	main	512,32 bits	26.89 ns	1.00	88 B	1.00

Add	PR	512,512 bits	26.70 ns	0.93	88 B	1.00
Add	main	512,512 bits	28.60 ns	1.00	88 B	1.00

Add	PR	512,8192 bits	93.30 ns	0.41	1048 B	1.00
Add	main	512,8192 bits	227.73 ns	1.00	1048 B	1.00

Add	PR	524288,128 bits	3,546.73 ns	0.32	65560 B	1.00
Add	main	524288,128 bits	10,926.60 ns	1.00	65560 B	1.00

Add	PR	524288,2048 bits	3,479.27 ns	0.31	65560 B	1.00
Add	main	524288,2048 bits	11,146.39 ns	1.00	65560 B	1.00

Add	PR	524288,32 bits	3,484.67 ns	0.32	65560 B	1.00
Add	main	524288,32 bits	10,955.92 ns	1.00	65560 B	1.00

Add	PR	524288,512 bits	3,546.81 ns	0.32	65560 B	1.00
Add	main	524288,512 bits	11,024.72 ns	1.00	65560 B	1.00

Add	PR	524288,8192 bits	3,615.26 ns	0.33	65560 B	1.00
Add	main	524288,8192 bits	11,059.42 ns	1.00	65560 B	1.00

Add	PR	8192,128 bits	84.49 ns	0.41	1048 B	1.00
Add	main	8192,128 bits	205.40 ns	1.00	1048 B	1.00

Add	PR	8192,2048 bits	128.64 ns	0.55	1048 B	1.00
Add	main	8192,2048 bits	233.31 ns	1.00	1048 B	1.00

Add	PR	8192,32 bits	85.97 ns	0.43	1048 B	1.00
Add	main	8192,32 bits	200.82 ns	1.00	1048 B	1.00

Add	PR	8192,512 bits	92.90 ns	0.41	1048 B	1.00
Add	main	8192,512 bits	226.46 ns	1.00	1048 B	1.00

Add	PR	8192,8192 bits	267.35 ns	1.04	1048 B	1.00
Add	main	8192,8192 bits	258.14 ns	1.00	1048 B	1.00

I've left some suggestions. Since this PR was stale for more than a quarter (it's our fault) and we snap for .NET 8 Preview 7 today, I am going to try to apply my suggestions myself and push the changes if the perf does not get worse. If I can't address my suggestions without regressing perf, I am going to merge as is.

adamsitnik · 2023-07-18T14:12:02Z

+ // Switching to managed references helps eliminating
+ // index bounds check...


The previous version of the code was eliminating index bound checks for left[i], as it was always used within a loop that was recognized by the JIT:
for(inti=0;i<left.Length;i++)longdigit=left[i]+carry;
This was not true for bits, because JIT did not know that bits.Length == left.Length + 1.
We could avoid using managed references for left and just keep using the old loop here and access bits via Unsafe.Add(ref managedRefToBits).
Suggested change
// Switching to managed references helps eliminating
// index bounds check...
// Switching to managed references helps eliminating
// index bounds check for both buffers.

adamsitnik · 2023-07-18T14:25:42Z

+ if (i < upperBound)
+ {
+ CopyTail(left, bits, unchecked((int)i));
+ }


this code seems identical with part of Add(ReadOnlySpan<uint> left, uint right, Span<uint> bits). If possible we should refactor it to helper method and enforce inlining, so the code is reused but there is no perf penalty.

adamsitnik · 2023-07-18T14:28:53Z

- long digit = left[i] + carry;
- Unsafe.Add(ref resultPtr, i) = (uint)digit;
- carry = digit >> 32;
+ for ( ; carry != 0 && i < upperBound; i++)


is there any particular reason why in other loops you have added a break for carry == 0 while here you moved the condition here?

Here was some differences in asm which is significant for "worst cases", but I need to redeploy this test environment to show.

…ate bound checks

…t inlined

speshuric · 2023-07-18T17:13:04Z

Since this PR was stale for more than a quarter

Sorry, I had to switch to some other activities on main job. But it seems now I can back to this.

adamsitnik

LGTM. @speshuric once again thank you for your contribution and apologies for the delay in review process!

ghost added community-contribution Indicates that the PR has been added by a community member area-System.Numerics labels Mar 26, 2023

build-analysisBot mentioned this pull request Mar 27, 2023
WasmTestOnBrowser-System.* test failures in CI #83655
Closed

tannergooding mentioned this pull request May 15, 2023
[Draft] Limit length of System.Runtime.BigInteger to int.MaxValue-32 bits #84901
Closed

Merge remote-tracking branch 'upstream/main' into bigintaddsub
128ac7d

adamsitnik self-assigned this Jul 18, 2023

adamsitnik added the tenet-performance Performance related issue label Jul 18, 2023

adamsitnik reviewed Jul 18, 2023
View reviewed changes

adamsitnik added 3 commits July 18, 2023 16:50

don't use managed references with Unsafe in cases when JIT can elimin…
dc7558a
…ate bound checks

reduce code duplication, refactor the logic to helper methods that ge…
d1b0a53
…t inlined

final polishing: comments & braces
beecaea

build-analysisBot mentioned this pull request Jul 18, 2023
LibraryImportGenerator.Unit.Tests crashed in CI #87951
Closed

adamsitnik approved these changes Jul 18, 2023
View reviewed changes

adamsitnik merged commit 2f843a8 into dotnet:mainJul 18, 2023

speshuric deleted the bigintaddsub branch July 19, 2023 00:54

ghost locked as resolved and limited conversation to collaborators Aug 18, 2023