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

Faster to_bignum_bigint #14742

Closed
wants to merge 4 commits into from
Closed

Faster to_bignum_bigint #14742

wants to merge 4 commits into from

Conversation

mitschabaude
Copy link
Contributor

@mitschabaude mitschabaude commented Dec 19, 2023

o1js: o1-labs/o1js#1338

This reimplements conversion of Rust field elements to Zarith bigints (Bigint.to_bignum_bigint). The conversion is used for the Hashable implementation of field elements, which previously was one of the main bottlenecks during witness generation, because plonk_constraint_system.ml uses a hashtable with field element keys.

The old conversion tested each individual bit of the Rust field. The new version tests 32 bits at once, and introduces a new bindings function test_uint32 for this purpose.

Empirically, this PR makes brings the witness generation time in a JS benchmark (Keccak) down by about 60%, from 1.7 to 0.7 seconds. The cost of hash calls, which used to be >50%, becomes negligible.

Note: IMO, this isn't the cleanest way to achieve the goal. It keeps the dependency on Zarith -- a separate, arbitrary-length bigint implementation, which is slow at least in the JS version -- just for support of a few methods like Hashable.hash. Ideally, we would implement hashing in Rust and JS and get rid of Zarith. However, that's a more impactful change and would have required more research. This here seems to be the minimal change that removes the bottleneck.

TODO: So far, only the JS backend implements test_uint32. Rust and Rust/wasm versions have to be added.

@mitschabaude
Copy link
Contributor Author

It turns out that the same function was optimized in rampup already: #14599
I tested and confirmed that it gives the same benefit as this PR. So for the sake of simplicity I'll close this.

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.

1 participant