Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimized BigInt implementation #9628

Merged
merged 7 commits into from May 13, 2021

Conversation

denisrosset
Copy link
Contributor

@denisrosset denisrosset commented May 11, 2021

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).

=====

This is a clean-up of #8932 with nicer history; see the original PR for much review discussion.

@scala-jenkins scala-jenkins added this to the 2.13.7 milestone May 11, 2021
@SethTisue SethTisue modified the milestones: 2.13.7, 2.13.6 May 11, 2021
@SethTisue SethTisue added prio:hi high priority (used only by core team, only near release time) library:base Changes to the base library, i.e. Function Tuples Try labels May 11, 2021
@denisrosset
Copy link
Contributor Author

denisrosset commented May 11, 2021

@SethTisue I guess the CI did not like my force push. The build https://travis-ci.com/github/scala/scala/jobs/504678083 is failing because of the unused "longEncoding" method, a problem that was fixed on the force push.

publishLocal works locally for the two commits d931366 and adaef2b

Should I open a new PR and close this one? (I'm now running publishLocal on my local builds -- I guess IntelliJ doesn't have the proper Ywarn-unused flags).

@SethTisue
Copy link
Member

SethTisue commented May 11, 2021

Travis likes it now. The Jenkins failures appear spurious and are now re-running.

(The "combined" check only requires every commit to be green on Jenkins... whatever went wrong on Travis-CI before won't matter.)

@denisrosset
Copy link
Contributor Author

@SethTisue I have no idea of the cause of the failures above ( 2e67ec1 not passing, but 7cc2a07 passing where I added only a comment line).

Is it due to the force-push?

Anyway, the two PR contain exactly the same code, and the history here is pretty clean.

  1. The commits a378d83 0ae2ff9 provide additional tests/benchmarks
  2. The commit d931366 prepares the code so that future invariants are enforced (probably with a temp. loss of performance)
  3. The commit adaef2b changes the BigInt internal representation, but keeps performing operations using underlying BigIntegers (temp. slower)
  4. The commit 2e67ec1 implements the optimized operations using the new internal representation
  5. ( 7cc2a07 is a comment to safe guard possible reuse of the longGcd implementation elsewhere )

@SethTisue
Copy link
Member

I don't know what happened, but Travis-CI went green when re-run.

@SethTisue SethTisue self-assigned this May 12, 2021
@SethTisue SethTisue removed their assignment May 12, 2021
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 12, 2021
@SethTisue
Copy link
Member

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

@SethTisue SethTisue merged commit 74c629e into scala:2.13.x May 13, 2021
@lrytz lrytz mentioned this pull request May 13, 2021
@SethTisue SethTisue removed the prio:hi high priority (used only by core team, only near release time) label May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:base Changes to the base library, i.e. Function Tuples Try release-notes worth highlighting in next release notes
Projects
None yet
3 participants