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

Use libj.math.BigInt for arithmetic #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

altr-benjamin
Copy link
Contributor

Java's built-in BigInteger class is a solid platform for building out programs that require the use of arbitrarily-sized numbers. However, when it comes to performance optimization, BigInteger can become problematic for the following reasons:

  • While it has been optimized over the years, it does not deliver optimal performance for certain mathematical operations
  • It only has immutable semantics (i.e. any mathematical operation on a BigInteger will allocate and return a new BigInteger), which leads to unavoidable memory pressure at scale.

This commit replaces BigInteger with libj's BigInt class, which uses a value-encoding scheme to deliver faster operations and mutable behavior which allows for improving memory footprint.

This change delivers a 45% improvement in the current benchmark suite on a 16GB Macbook M1 Pro:

Benchmark                   Mode  Cnt      Score      Error  Units
FF3CipherPerf.testEncrypt  thrpt    5  97629.798 ± 2972.249  ops/s

Benchmark                   Mode  Cnt       Score      Error  Units
FF3CipherPerf.testEncrypt  thrpt    5  178592.429 ± 3097.461  ops/s

This commit introduces [email protected] as a project dependency.

Java's built-in `BigInteger` class is a solid platform for building
out programs that require the use of arbitrarily-sized numbers. However,
when it comes to improving performance, `BigInteger` can become problematic
for the following reasons:

 - While it has been optimized over the years, it does not deliver optimal
   performance for certain mathematical operations
 - It only has immutable semantics (i.e. any mathematical operation on a
   `BigInteger` will allocate and return a *new* `BigInteger`), which leads
   to unavoidable memory pressure at scale.

This commit replaces `BigInteger` with libj's `BigInt` class, which uses a
value-encoding scheme to deliver faster operations and mutable behavior which
allows for improving memory footprint.

This change delivers a 45% improvement in the current benchmark suite on
a 16GB Macbook M1 Pro:

```
Benchmark                   Mode  Cnt      Score      Error  Units
FF3CipherPerf.testEncrypt  thrpt    5  97629.798 ± 2972.249  ops/s

Benchmark                   Mode  Cnt       Score      Error  Units
FF3CipherPerf.testEncrypt  thrpt    5  178592.429 ± 3097.461  ops/s
```

This commit introduces
[`libj.math.BigInt`](https://mvnrepository.com/artifact/org.libj/math/0.6.8)
as a project dependency.
@altr-benjamin altr-benjamin marked this pull request as ready for review November 25, 2024 14:31
@bschoening
Copy link
Member

@altr-benjamin What's the need for maven.repository.redhat.com in build.gradle.kts? libj is available MavenCentral (https://mvnrepository.com/artifact/org.libj/math).

@altr-benjamin
Copy link
Contributor Author

@altr-benjamin What's the need for maven.repository.redhat.com in build.gradle.kts? libj is available MavenCentral (https://mvnrepository.com/artifact/org.libj/math).

libj:utils pulls in diff_match_patch which lives in Redhat GA.

Without maven("https://maven.repository.redhat.com/ga/"):

image

@bschoening
Copy link
Member

bschoening commented Nov 30, 2024

@altr-benjamin

  1. Github repo for diff-match-patch states: "This repository has been archived by the owner on Aug 4, 2024. It is now read-only." From a security perspective, that's likely a show-stopper.

Does libj:match mention this dependency? Do they have a plan going forward to replace it?

I filed #59 for this.

  1. With libj on a MacBook M2, I saw ~20% better performance vs the 40% you reported. What's libmathj_x86_64.dylib which didn't load on my test?

(original)
Benchmark Mode Cnt Score Error Units
FF3CipherPerf.testEncrypt thrpt 5 101072.369 ± 52038.015 ops/s

(with libj:math patch)
// Warmup Iteration 1: Not found: /libmathj_x86_64.dylib
// Starting without JNI bindings

Benchmark Mode Cnt Score Error Units
FF3CipherPerf.testEncrypt thrpt 5 121699.745 ± 55075.206 ops/s

@altr-benjamin
Copy link
Contributor Author

@bschoening In order:

  1. Thanks for opening that! I am going to follow that issue, and if there's anything needed in that ecosystem we'd be open to committing upstream as well. FWIW it would appear that the author of the math library is heavily or solely involved in the libj project as a whole.
  2. libj:math makes low-level implementations and native bindings available and selects the optimal binding based on circumstances. It would appear in your case that the library is failing to identify the proper shared lib (it looks to be the wrong architecture) and falls back to plain-ol' Java code which the author claims is JIT-friendly. I can't speak to your toolchain specifically, but using IntelliJ and reloading Gradle collected the proper dependency for me.

@bschoening
Copy link
Member

bschoening commented Dec 21, 2024

@altr-benjamin seems it may take a while to resolve the unnecessary diff_match_patch dependency. Given this, I'm considering making a new release of java-fpe which would include the String the char array changes. Let me know if you would find that useful.

The discussion on libj has highlighted some of the downsides of hardwiring in lesser known 3rd party libraries maintained by a sole individual. I'm wondering if we can make this pluggable with a factory provider, the way SLF4J works.

@altr-benjamin
Copy link
Contributor Author

@bschoening if you'd like to cut a new release with the char array changes that's fine by me -- any perf improvement is welcome there.

Good point and interesting comment on the pluggable pattern! IMO the big difference b/w BigInteger and libj is the mutability, but I'd be happy to take that to the drawing board and see if it could be abstracted out enough to be workable.

@bschoening
Copy link
Member

@altr-benjamin BigInteger is immutable for thread safety. But I don't see that as needed in a Feistel cipher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants