jellyfish icon indicating copy to clipboard operation
jellyfish copied to clipboard

chore: overhead free type conversion for `load_poly_to_gpu`

Open mrain opened this issue 1 year ago • 1 comments

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 Pending section in CHANGELOG.md
  • [ ] Re-reviewed Files changed in the GitHub PR explorer

mrain avatar Mar 14 '24 02:03 mrain

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

alxiong avatar Mar 14 '24 02:03 alxiong

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

ggutoski avatar Apr 01 '24 19:04 ggutoski