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

Improve lagrange retrieval performance & remove unsafes #16272

Merged

Conversation

volhovm
Copy link
Member

@volhovm volhovm commented Oct 22, 2024

  1. This creates a binding function _lagrange_commitments_whole_domain that is to be used instead of calling _lagrange_commitment (that retrieves a single base) domain_size times.
    • The optimisation saves up about 100ms on every prove call on my local machine if I remember correctly, or less -- in the end it cuts about 0.5m or 6% time on the cluster:
    • Benchmark improvement (on a fast-ish cluster) for _build/default/src/app/cli/src/mina.exe transaction-snark-profiler --zkapps --k 1 --max-num-updates 4 --min-num-updates 2:
- NEW
  - 6.231m
  - 6.378m
  - 6.381m
- OLD:
  - 6.843m
  - 6.753m
  - 6.743m
  1. Additionally I removed some unsafes that are not anymore necessary since SRS is now wrapped in a Mutex (which should be RWLock really).

Caveat: this change plus some amount of debug printing invokes a completely different deadlocking bug with me where the verifier process no longer starts. This seems to be triggered by the code becoming faster, so some race condition is triggered.

I managed to reproduce the deadlock over compatible too: o1-labs/proof-systems#2728

@volhovm volhovm requested a review from a team as a code owner October 22, 2024 13:13
@volhovm
Copy link
Member Author

volhovm commented Oct 22, 2024

!ci-build-me

@volhovm volhovm force-pushed the volhovm/16112-lagrange-bases-one-by-one-optimisation branch from da94561 to d9dbb02 Compare October 22, 2024 18:06
@volhovm
Copy link
Member Author

volhovm commented Oct 22, 2024

!ci-build-me

1 similar comment
@volhovm
Copy link
Member Author

volhovm commented Oct 23, 2024

!ci-build-me

@volhovm volhovm force-pushed the volhovm/16112-lagrange-bases-one-by-one-optimisation branch from d9dbb02 to f5c89f3 Compare October 23, 2024 16:26
@volhovm
Copy link
Member Author

volhovm commented Oct 23, 2024

!ci-build-me

@querolita
Copy link
Member

Caveat: this change plus some amount of debug printing invokes a completely different deadlocking bug with me where the verifier process no longer starts. This seems to be triggered by the code becoming faster, so some race condition is triggered.

Do you mean we are paying ~100ms less but now we have a new race condition because a thread becomes too fast and accesses a resource? Then I am not sure we should merge this before we solve the other problem first 🤔

@volhovm
Copy link
Member Author

volhovm commented Oct 24, 2024

@querolita

  1. I ran tests on this branch many times and they all succeed. So I don't think there's anything in this thread that is actually a problem.
  2. By adding some printing I managed to reproduce the deadlock even over compatible. This makes me suspicious that the problem is with printing on the rust side that breaks some pipes with the verifier client, and it's not lagrange related at all.
  3. I'm happy to take responsibility for any issues if they come up

t
-> int
-> Pasta_bindings.Fq.t Kimchi_types.or_infinity Kimchi_types.poly_comm
array = "caml_fp_srs_lagrange_commitments_whole_domain"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit confusing that the one using Pasta_bindings.Fq.t is called caml_fp_srs_lagrange... , but I see this is the format being followed across this file so I suppose that notation is expected.

@volhovm
Copy link
Member Author

volhovm commented Oct 24, 2024

!ci-run-nightly

@volhovm
Copy link
Member Author

volhovm commented Oct 24, 2024

!ci-nightly-me

@volhovm
Copy link
Member Author

volhovm commented Oct 24, 2024

I will check if nightlies pass first, and merge if they do. I'm fairly confident it's OK but I'm also not in a hurry.

@volhovm
Copy link
Member Author

volhovm commented Oct 24, 2024

Nightlies seem to pass (I did it twice) except for "Publish Mina for Bullseye" which fails due to some unrelated debian thing.

Merging.

@volhovm volhovm merged commit 996b95b into compatible Oct 24, 2024
45 checks passed
@volhovm volhovm deleted the volhovm/16112-lagrange-bases-one-by-one-optimisation branch October 24, 2024 16:22
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.

None yet

2 participants