jellyfish
jellyfish copied to clipboard
chore: overhead free type conversion for `load_poly_to_gpu`
Description
This PR avoids a memory copy in load_poly_to_gpu. However it contains unsafe rust code.
Before:
Start: KZG10::Setup with prover degree 4194304 and verifier degree 1
...
Start: Type Conversion: ark->ICICLE: Scalar
End: Type Conversion: ark->ICICLE: Scalar ......................................17.305ms
...
After:
Start: KZG10::Setup with prover degree 4194304 and verifier degree 1
...
Start: Type Conversion: ark->ICICLE: Scalar
End: Type Conversion: ark->ICICLE: Scalar ......................................160ns
...
It doesn't apply to affine point conversion because of two different underlying struct reprs.
Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why.
- [ ] Targeted PR against correct branch (main)
- [ ] Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
- [ ] Wrote unit tests
- [ ] Updated relevant documentation in the code
- [ ] Added a relevant changelog entry to the
Pendingsection inCHANGELOG.md - [ ] Re-reviewed
Files changedin the GitHub PR explorer
This is pretty impressive speed-up, but you pushed the instance to be 2^22 to make the gap meaningful, for much smaller instances (which is our case), the gap is so small, (correct?)
- I have no objection of using this unsafe, since it's very limited scope, and easy to reason about. As long as we add some tests to constantly watching for the "same underlying repr" assumption is correct, I'm comfortable. (ICICLE is using tones of unsafe anyway.)
- I vote we turn this into a draft PR and archive it (instead of merging it). and use it in the future when the instance size become so large that conversion time is a bottleneck.
wdyt? @chancharles92 @mrain
- I have no objection of using this unsafe, since it's very limited scope, and easy to reason about. As long as we add some tests to constantly watching for the "same underlying repr" assumption is correct, I'm comfortable. (ICICLE is using tones of unsafe anyway.)
We could add a test to enforce this, no? See https://github.com/EspressoSystems/jellyfish/pull/516#discussion_r1546760736
- I vote we turn this into a draft PR and archive it (instead of merging it). and use it in the future when the instance size become so large that conversion time is a bottleneck.
I think I disagree. An archived PR will experience bit rot and get forgotten. Is your concern only about the unsafe code? Perhaps that concern could be mitigated with a stricter test described above.