Optimized BigInt implementation

[denisrosset]

This augments the BigInt class with a Long field, so that small integers can be handled without allocating a java.math.BigInteger.

The main motivation is to make BigInt a "safe integer" type with a reasonable performance profile in the case it is mainly used to store Long-sized integers; whereas the overhead of the introduced special case is dwarfed by the java.math.BigInteger machinery.

To maintain binary compatibility even if BigInt is a final class, I added a main constructor parameter of type Long; that main constructor is private. The original constructor is now a secondary constructor taking a single BigInteger argument. Logic inside the constructor decides whether to store the value as a BigInteger or Long argument.

This introduces a small memory overhead when BigInt stores a BigInteger (but in turn, BigInteger is a pretty mighty class which allocates an array).

Points to be discussed:

• MiMA doesn't complain: but the val bigInteger: BigInteger constructor argument is now a def bigInteger: BigInteger body method. Is that OK?

• This needs to be benchmarked. What would be a relevant benchmark in the Scala community build?

• The GCD optimization rests on a Long GCD code lifted from the Spire library, which is under the MIT License. Is such a code integration permitted by the Scala CLA?

• The Java API documentation allows for big integers with a bit length bigger than what an Int can store. I'm using bitLength liberally in the new implementation, I don't know how the code would behave in these cases. On the other hand, the old code calls bitLength a few times before having code to handle the "overly big BigInteger speical case". Shouldn't bitLength throw an exception beforehand (the API specifies that methods should throw an exception instead of returning an invalid value, but this point isn't clear). What should we do there? (It's interesting to find this type of undefined behaviour in the Java API).

[johnynek]

This is great! Thanks for tackling this. I’m on my phone but a few questions:

Are you sure there is test coverage for all these changes? I can imagine we need to be more careful and really exercise all the branches now.

Can we commit any benchmarks to compare to BigInteger and java.lang.Long? The goal, I imagine here is be only a bit slower than a boxed Long but have the ability to scale up. Would be nice to demonstrate that in benchmarks (sorry if I overlooked them in this PR).

[denisrosset]

Ok, MiMa complains about

[error] scala-library: Failed binary compatibility check against org.scala-lang:scala-library:2.13.0! Found 1 potential problems (filtered 412)

[error]  * method this(Long)Unit in class scala.math.BigInt does not have a correspondent in other version

even though that (new) constructor is private. Is that a problem in practice?

[sjrd]

Ok, MiMa complains about

[error] scala-library: Failed binary compatibility check against org.scala-lang:scala-library:2.13.0! Found 1 potential problems (filtered 412)

[error]  * method this(Long)Unit in class scala.math.BigInt does not have a correspondent in other version

even though that (new) constructor is private. Is that a problem in practice?

That's not a problem in practice. The constructor is not accessible from Scala from a different file (although it would be accessible from Java), so it cannot break linking across different artifacts.

* MiMA doesn't complain: but the `val bigInteger: BigInteger` constructor argument is now a `def bigInteger: BigInteger` body method. Is that OK?

I believe that's OK. It's not source compatible (the member's not stable anymore, which means you cannot import its members without storing it in a val), but it is binary compatible.

[denisrosset]

New encoding which caches a BigInteger instance after request.

[johnynek]

I like the caching, noticed something that looks like a bug.

[dwijnand]

(We use [nomerge] to indicate PRs with changes that can't forward-merge. I added the "WIP" label instead, but that practice could be redundant with draft PRs, and being able to convert PRs into draft PRs... Anyways.)

[denisrosset]

Changes

• I've written BigInt benchmarks. They are available in a separate branch, so I can switch between implementations. See https://github.com/denisrosset/scala/tree/bigint-benchmarks

• I've addressed the comments above. For the use of cache or not, I've made a new branch, which is not part of this PR, but is benchmarked below.

Help needed

• Ideas for additional relevant benchmarks? Is it micro-op benchmarking worthwhile (i.e. test method by method)?

• Is there any way to automate running a bunch of JMH tests and collect the results?

• The standard deviation is all over the place; I'm running tests on a laptop (Ubuntu 20.04), from the command line. Any advice?

I'm using the following JMH annotations. @BenchmarkMode(Array(Mode.AverageTime)) @Fork(2) @Threads(1) @Warmup(iterations = 10) @Measurement(iterations = 10) @OutputTimeUnit(TimeUnit.NANOSECONDS) @State(Scope.Benchmark)

• Any advice to reduce the feedback cycle time? Right now, one benchmark takes around 10 minutes to run which is not ideal to iterate.

• Conversely, any advice for the parameters for the final comparison between options? I.e. evidence that I would present in this PR? I can afford then to wait a bit more.

Preliminary benchmark results

I'm comparing:

1. the original 2.13 BigInt implementation in b499fcf

2. this PR denisrosset@0f9c5f9

3. a variant that uses BigInt.apply and thus the cache denisrosset@60f69d8

For the RSA benchmark, I get:

1. 8564.509 ±(99.9%) 293.898 ns/op [Average]
2. 8543.954 ±(99.9%) 38.654 ns/op [Average]
3. 8689.300 ±(99.9%) 143.148 ns/op [Average]

For the factorial benchmark:

Data of the factorial benchmark below:

1. Latest 2.13.x library

Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    91.325 ±  0.783  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20   189.208 ±  0.942  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   286.799 ±  3.006  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   400.816 ±  5.075  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   520.375 ± 13.439  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   651.065 ±  5.901  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   756.722 ± 14.020  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   908.575 ± 15.089  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20  1034.421 ± 20.981  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1182.123 ± 14.954  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1309.791 ±  8.394  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1432.263 ± 24.768  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1553.675 ± 13.574  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1702.169 ± 11.971  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  1889.631 ± 24.950  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2027.780 ± 26.827  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2226.690 ± 40.736  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2423.215 ± 39.091  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2587.199 ± 51.026  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2755.073 ± 40.325  ns/op

2. Optimized BigInt without cache

Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    51.850 ±  0.719  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20    97.546 ±  1.250  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   142.426 ±  2.590  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   186.384 ±  2.220  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   377.740 ±  4.264  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   536.705 ±  7.151  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   675.759 ±  9.551  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   839.809 ±  4.489  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20   955.144 ± 30.492  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1080.100 ± 11.415  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1201.680 ± 21.882  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1429.358 ± 18.297  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1548.369 ± 10.125  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1711.400 ± 21.597  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  1947.580 ± 70.605  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2016.584 ± 35.509  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2198.158 ± 12.886  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2454.478 ± 67.294  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2703.225 ± 83.176  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2875.966 ± 87.845  ns/op

3. Optimized BigInt with cache

Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    50.208 ±   1.377  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20   105.166 ±   1.669  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   154.791 ±   2.989  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   202.301 ±   2.199  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   389.317 ±   5.932  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   529.150 ±   6.468  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   687.965 ±  15.501  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   832.886 ±  15.126  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20  1004.644 ±  18.778  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1148.712 ±  25.158  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1240.980 ±  19.981  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1475.280 ±  60.774  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1687.045 ±  55.694  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1744.787 ±  44.528  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  2004.772 ±  52.582  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2154.762 ±  76.210  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2381.689 ±  66.024  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2510.976 ±  37.217  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2810.998 ±  60.709  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2976.557 ± 113.094  ns/op

[denisrosset]

Caching in BigInt.apply was calling the wrong constructor. After correction, the version using the cache seems faster (it is within the standard deviation though).

Latest 2.13.x library
Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    91.325 ±  0.783  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20   189.208 ±  0.942  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   286.799 ±  3.006  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   400.816 ±  5.075  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   520.375 ± 13.439  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   651.065 ±  5.901  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   756.722 ± 14.020  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   908.575 ± 15.089  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20  1034.421 ± 20.981  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1182.123 ± 14.954  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1309.791 ±  8.394  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1432.263 ± 24.768  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1553.675 ± 13.574  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1702.169 ± 11.971  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  1889.631 ± 24.950  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2027.780 ± 26.827  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2226.690 ± 40.736  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2423.215 ± 39.091  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2587.199 ± 51.026  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2755.073 ± 40.325  ns/op

Optimized BigInt without cache
Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    56.005 ±  2.844  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20    99.144 ±  0.638  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   146.020 ±  0.859  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   190.778 ±  2.982  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   371.584 ±  6.375  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   536.971 ±  5.861  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   668.471 ±  7.294  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   842.652 ± 11.786  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20   941.192 ± 12.239  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1099.328 ± 10.535  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1250.473 ± 19.429  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1402.944 ± 34.437  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1553.778 ± 23.653  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1670.973 ± 23.180  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  1915.087 ± 54.379  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2048.740 ± 47.212  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2217.776 ± 23.843  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2397.801 ± 43.543  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2621.795 ± 40.232  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2828.747 ± 58.761  ns/op

Optimized BigInt with cache
Benchmark                           (size)  Mode  Cnt     Score    Error  Units
BigIntFactorialBenchmark.factorial       5  avgt   20    47.680 ±   0.416  ns/op
BigIntFactorialBenchmark.factorial      10  avgt   20    91.619 ±   0.746  ns/op
BigIntFactorialBenchmark.factorial      15  avgt   20   152.791 ±   0.571  ns/op
BigIntFactorialBenchmark.factorial      20  avgt   20   205.353 ±   2.425  ns/op
BigIntFactorialBenchmark.factorial      25  avgt   20   388.793 ±  13.057  ns/op
BigIntFactorialBenchmark.factorial      30  avgt   20   520.664 ±   9.426  ns/op
BigIntFactorialBenchmark.factorial      35  avgt   20   693.699 ±  18.214  ns/op
BigIntFactorialBenchmark.factorial      40  avgt   20   835.729 ±  40.293  ns/op
BigIntFactorialBenchmark.factorial      45  avgt   20   988.396 ±  12.616  ns/op
BigIntFactorialBenchmark.factorial      50  avgt   20  1099.307 ±  17.729  ns/op
BigIntFactorialBenchmark.factorial      55  avgt   20  1208.516 ±  43.706  ns/op
BigIntFactorialBenchmark.factorial      60  avgt   20  1416.259 ±  20.404  ns/op
BigIntFactorialBenchmark.factorial      65  avgt   20  1548.893 ±  42.810  ns/op
BigIntFactorialBenchmark.factorial      70  avgt   20  1750.144 ±  28.317  ns/op
BigIntFactorialBenchmark.factorial      75  avgt   20  1873.494 ±  34.342  ns/op
BigIntFactorialBenchmark.factorial      80  avgt   20  2050.726 ±  17.714  ns/op
BigIntFactorialBenchmark.factorial      85  avgt   20  2250.785 ±  28.706  ns/op
BigIntFactorialBenchmark.factorial      90  avgt   20  2485.059 ± 130.190  ns/op
BigIntFactorialBenchmark.factorial      95  avgt   20  2594.462 ±  48.606  ns/op
BigIntFactorialBenchmark.factorial     100  avgt   20  2845.801 ±  46.668  ns/op

[lrytz]

* Is there any way to automate running a bunch of JMH tests and collect the results?

Mabye https://github.com/lightbend/benchdb could be helpful? It's very new.

* The standard deviation is all over the place; I'm running tests on a laptop (Ubuntu 20.04), from the command line. Any advice?

Most errors seem to be around 1%, which seems reasonable no?

scala/scala-dev#338 has information, probably more than you need :-) Generally, I could imagine results being less stable on laptops because they usually don't have enough cooling to keep the CPU frequency stable.

* Any advice to reduce the feedback cycle time? Right now, one benchmark takes around 10 minutes to run which is not ideal to iterate.

Hmm, 10 minutes is not that much for a benchmark, but obviously annoying if you can't hand it off to a different machine. In fact you might actually want to do more than 2 forks if you need more results. Did you check (manually, by looking at the numbers) that 10 warmup iterations are enough?

Maybe you can do a log scale on the sizes that you measure and thin it out.

[johnynek]

I made the same comment over and over: why are we doing new BigInt(long) rather than BigInt.apply(long) and seeing if we can find it in the cache.

[denisrosset]

@lrytz

Mabye https://github.com/lightbend/benchdb could be helpful? It's very new.

So new, that I think I can't access it! Is the project still private?

Most errors seem to be around 1%, which seems reasonable no?

I'm seeing the std dev magnitude fluctuating a lot between benchmark instances, so I was wondering if it comes from JIT instability, or CPU performance instability (throttling etc)

scala/scala-dev#338 has information, probably more than you need :-) Generally, I could imagine results being less stable on laptops because they usually don't have enough cooling to keep the CPU frequency stable.

I'll move the benchmarks to a dedicated machine, and will use two tips: disable HT and Turbo Boost.

Hmm, 10 minutes is not that much for a benchmark, but obviously annoying if you can't hand it off to a different machine. In fact you might actually want to do more than 2 forks if you need more results. Did you check (manually, by looking at the numbers) that 10 warmup iterations are enough?

I can reduce the number of test cases now. How many forks do people usually ask for? I'm cargo culting the JMH parameters for now.

I guess to decide on the number of warmup iterations, I should set a large number like 50, and then check manually the convergence (average, std dev) in the output?

And also perform a larger number of forks, and compare the behaviour across forks?

And it's time to write a small tool to interpret/plot the benchmark results (I'm cut and pasting things in OpenOffice for now).

Final question: for such microbenchmarks, is there a difference when the test runs for longer, in that the benchmark will start to include the code memory pressure? Imagine two variants, V1 and V2, where V1 doesn't allocate, and V2 allocates a bit. Then the performance profile in the benchmark will depend on whether the benchmark runs until V2 triggers GC, right? Is there an accepted standard on how to benchmark this fairly?

(I'm now reading this: https://shipilev.net/talks/devoxx-Nov2013-benchmarking.pdf but I'd welcome links to resources/books. Any information I get, I'll submit a scala/scala PR to the bench project README as to not lose that info.)

[linasm]

I'm seeing the std dev magnitude fluctuating a lot between benchmark instances, so I was wondering if it comes from JIT instability, or CPU performance instability (throttling etc)

When benchmarking on a laptop, make sure you have Turbo Boost disabled:
https://askubuntu.com/questions/619875/disabling-intel-turbo-boost-in-ubuntu

[lrytz]

So new, that I think I can't access it! Is the project still private?

oops 😄 i'll let you know!

it's time to write a small tool to interpret/plot the benchmark results

that's exactly what benchdb would do

How many forks do people usually ask for?
check manually the convergence (average, std dev) in the output?

That's what I'd do, yeah. I personally don't have a lot of experience with bechmarking and JMH. Maybe @retronym can debunk some of the bad advice I'm giving here.

performance profile in the benchmark will depend on whether the benchmark runs until V2 triggers GC, right?

Yes, I think that's right. Quantifying the benefit of reducing the allocation rate of an implementation is extremely difficult because the effect on GC is unpredictable. There can even be unexpected side-effects, like a lot of short-lived allocation can be cheaper (handled by minor GCs) compared to a long-lived cache that gets promoted to old gen.

I assume JMH would do its best to force a GC before starting to measure a benchmark? I don't know.

There's also the option to enable a profiler while running benchmarks https://hg.openjdk.java.net/code-tools/jmh/file/b6f87aa2a687/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_35_Profilers.java#l52, -prof gc should give some insight.

[lrytz]

Mabye lightbend/benchdb could be helpful? It's very new.

So new, that I think I can't access it! Is the project still private?

The benchdb repository is public now, please take a look and feel free to give it a try! It was developed and used by @szeiger while he was working on the new Vector implementation that landed in 2.13.2 (#8534).

[johnynek]

Denis: thanks for the reply about when to use new BigInt vs BigInt.apply. I would imagine though that the calculus is different now. Since the Long wrapping versions are also much more likely to be smaller values, and comparing Long to an Int (all that’s needed for checking if it is cached) is so cheap, my guess is that for the Long returning branches using caching will often be a win for code that is using many smaller values.

[dwijnand]

@denisrosset could you rebase and squash the commits, please? Other than that would you say this is ready for merge?

[johnynek]

a few more comments.

Thanks a lot for pushing on this. It would be really amazing to improve the performance of BigInt.

[denisrosset]

@SethTisue

As far as I see, we had the following concerns:

@Ichoran were all your concerns addressed?

Haven't seen a reply, but I don't see any blockers.

@denisrosset there are some conversations above not yet marked "resolved", can you check on those?

I've looked at all conversations, and made the changes.

@denisrosset earlier you mentioned that you had benchmarking code in a separate branch — is that all part of the PR now?
as for further benchmarking of details, I suggest that be considered possible-future-work. clearly this is a win overall

I did not merge benchmarking code; it's been a while! I'd be happy to have a second look at it later.

@denisrosset / all: was the "bit length bigger than what an Int can store" question answered? is it a blocker?

I will check on the license question about bringing in code from Spire
(licenses are compatible, I just need to make sure we have the right notices in the right places)

Are we OK?

No other concerns that I can see. This is nearly ready for merge for 2.13.5. We can do this!

Agreed!

---

Code coverage-wise (by the ScalaCheck test):

• hashCode: the "big" case isn't checked, but it calls bigInteger.## as before,
• isValidFloat/isValidDouble: the case for bitLen > 24 / bitLen > 53 isn't checked, but the code is the same as before,
• the until/to methods aren't covered, but they haven't changed,
• I've introduced dummy private implicit conversion methods to deactivate implicit conversions in the BigInt code itself, and of course they aren't called.

For the testing, are we confident that the BigInt code is exercised by external code (crypto? math?)?

[denisrosset]

We still need the MiMa filter, as we add a new (private) constructor to a class.

[johnynek]

Just a couple more comments.

[SethTisue]

@denisrosset we may build 2.13.6 as soon as Mon May 17, so there's still a chance this could make it in. care to respond to Oscar's latest review round?

[SethTisue]

For the testing, are we confident that the BigInt code is exercised by external code (crypto? math?)?

There is presumably some usage in the community build, which will run after the PR is merged.

The license question is still on my plate; I'll get to it this week.

Note that before final merge this will need a rebase so that every commit passes CI. Simply squashing to a single commit would be okay, but @denisrosset if you want to preserve a little more history than that, that's fine too as long as every commit passes CI.

[denisrosset]

@SethTisue If we have the time, I'll rebuild a series of commits in another branch to preserve some history. Otherwise, I'm happy with a squash.

Concerning the license: there is another (and likely faster!) implementation of the Long gcd in Guava, which is Apache 2-licensed. Should we switch?

[SethTisue]

Concerning the license: there is another (and likely faster!) implementation of the Long gcd in Guava, which is Apache 2-licensed. Should we switch?

Perfect! Yes

[denisrosset]

@johnynek @SethTisue The code in this PR and #9628 are now exactly the same, except that #9628 has a cleaner history which should make things easier to review.

[SethTisue]

Let's merge #9628 on Thursday unless there is further review feedback.