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

feat: use assign_regions api to speed up the witness assignment #24

Closed
wants to merge 29 commits into from

Conversation

alannotnerd
Copy link
Contributor

@alannotnerd alannotnerd commented May 28, 2023

See #23

Bechmarking

For each phase (in AMD EPYC 7702 64-Core Processor x 128 threads):

before

ASSIGNMENT_TYPE: default Elapsed: 22.257506277s

after:

[2023-06-16T06:58:10Z INFO  poseidon_circuit::hash] hash table parallel version took 445.565348ms
[2023-06-16T07:03:29Z INFO  poseidon_circuit::hash] final state took 110.563839ms

around 556ms

@kunxian-xia kunxian-xia self-requested a review June 11, 2023 03:19
@alannotnerd alannotnerd changed the base branch from main to scroll-dev-0408 June 11, 2023 14:20
Copy link
Contributor

@kunxian-xia kunxian-xia left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

Cargo.toml Outdated Show resolved Hide resolved
src/hash.rs Outdated Show resolved Hide resolved
src/hash.rs Outdated Show resolved Hide resolved
src/hash.rs Show resolved Hide resolved
src/hash.rs Outdated Show resolved Hide resolved
src/hash.rs Show resolved Hide resolved
@kunxian-xia
Copy link
Contributor

@noel2004 Is it true that each ((inp, control), check) occupies exactly one row in the fill_hash_tbl_body region?

for (i, ((inp, control), check)) in inputs_i.zip(controls_i).zip(checks_i).enumerate() {

src/hash.rs Outdated Show resolved Hide resolved
src/hash.rs Outdated Show resolved Hide resolved
src/hash.rs Outdated Show resolved Hide resolved
@noel2004
Copy link
Member

@noel2004 Is it true that each ((inp, control), check) occupies exactly one row in the fill_hash_tbl_body region?

for (i, ((inp, control), check)) in inputs_i.zip(controls_i).zip(checks_i).enumerate() {

Yes. Each entry occupies one row.

@kunxian-xia
Copy link
Contributor

kunxian-xia commented Jun 12, 2023

There remains two problems to be solved:

  • parallelize all these permute to get final state (10s out of 60s),
  • reduce 1st phase of fill_hash_tbl_body_partial to 0.

@kunxian-xia
Copy link
Contributor

@noel2004 Is it true that each ((inp, control), check) occupies exactly one row in the fill_hash_tbl_body region?

for (i, ((inp, control), check)) in inputs_i.zip(controls_i).zip(checks_i).enumerate() {

Yes. Each entry occupies one row.

@alannotnerd The region height for each fill_hash_tble_body_partial is equal to data.len().

@kunxian-xia kunxian-xia self-requested a review June 13, 2023 10:21
@kunxian-xia kunxian-xia requested review from noel2004 and removed request for noel2004 June 13, 2023 10:23
@kunxian-xia kunxian-xia changed the title feat: use assign_regions api to speed up hash function feat: use assign_regions api to speed up the witness assignment Jun 13, 2023
@alannotnerd
Copy link
Contributor Author

alannotnerd commented Jun 13, 2023

Latest benchmark:

[2023-06-13T11:42:40Z INFO  halo2_proofs::circuit::floor_planner::single_pass] 25 sub_regions of hash table 2nd pass synthesis took 966.003307ms
[2023-06-13T11:42:40Z INFO  poseidon_circuit::hash] hash table parallel version took 972.602083ms
[2023-06-13T11:42:40Z INFO  halo2_proofs::circuit::floor_planner::single_pass] 24 sub_regions of permute state 2nd pass synthesis took 41.37005ms
[2023-06-13T11:42:40Z INFO  poseidon_circuit::hash] final state took 834.965283ms

@kunxian-xia
Copy link
Contributor

kunxian-xia commented Jun 13, 2023

On a machine with 128 threads, we get the following logs:

[2023-06-13T09:57:52Z INFO  halo2_proofs::circuit::floor_planner::single_pass] 128 sub_regions of hash table 2nd pass synthesis took 403.969017ms
[2023-06-13T09:57:52Z INFO  poseidon_circuit::hash] hash table parallel version took 456.107107ms

The running time of fill_hash_tbl_body part is reduced to about 0.5s.

@kunxian-xia kunxian-xia added the enhancement New feature or request label Jun 13, 2023
@kunxian-xia
Copy link
Contributor

We got the following stats when testing in scroll-zkevm (tested on a machine with 128 threads):

[2023-06-16T06:58:10Z INFO  poseidon_circuit::hash] hash table parallel version took 445.565348ms
[2023-06-16T07:00:47Z INFO  poseidon_circuit::hash] hash table parallel version took 452.829501ms
[2023-06-16T07:03:29Z INFO  poseidon_circuit::hash] hash table parallel version took 490.576667ms
[2023-06-16T06:58:10Z INFO  poseidon_circuit::hash] final state took 105.097895ms
[2023-06-16T07:00:47Z INFO  poseidon_circuit::hash] final state took 103.493175ms
[2023-06-16T07:03:29Z INFO  poseidon_circuit::hash] final state took 110.563839ms

@noel2004
Copy link
Member

Has been merged into main

@noel2004 noel2004 closed this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up poseidon circuit's witness assignment using assign_regions API
3 participants