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

Investigate improvements for assign_region #155

Open
pinkiebell opened this issue Feb 28, 2023 · 5 comments
Open

Investigate improvements for assign_region #155

pinkiebell opened this issue Feb 28, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@pinkiebell
Copy link

At this moment, the SimpleFloorPlanner used in zkevm-circuits will call the closure for assign_region twice.
It might makes sense to deduplicate this call in halo2 because there a few compute heavy loops inside zkevm-circuits that not only does assignments but also data mapping / transformation and right now, doing the work twice. We can gain significant speedups by either
i) modifying zkevm-circuits to do the heavy work outside the closure and reference it instead
ii) find a way to avoid calling more than 1 times in halo2 - this generally improves performance for any circuit/crate

@pinkiebell
Copy link
Author

@jonathanpwang
Copy link

I have a PR for this here: halo2-ce#13
You can cherry-pick the last 2 commits and test it out.

@pinkiebell
Copy link
Author

I have a PR for this here: halo2-ce#13
You can cherry-pick the last 2 commits and test it out.

I've tested that and while it makes the synthesize step faster (2x) it greatly increases the degree: privacy-scaling-explorations/zkevm-chain@f16e43d .

Maybe we can store all information from assignments and then do the shaping instead calling it multiple times?

@CPerezz
Copy link
Member

CPerezz commented Mar 3, 2023

I've tested that and while it makes the synthesize step faster (2x) it greatly increases the degree: privacy-scaling-explorations/zkevm-chain@f16e43d .

I don;t think that's an issue. While we keep the expr degree low (that's of course the case as it isn't touched) sacrifice 1 degree in the MockProver is not an issue at all IMO (Specially considering that this Layouter should never be used for final circuits/benchmarks).

Which is your proposal @pinkiebell ? I thought it's not a big deal to get a bigger degree (specially since we don't need a setup with the MockProver.

@pinkiebell
Copy link
Author

It would be a great performance improvement for real & mock prover alike to improve halo2 to not calling these closures from assign_region etc. twice. Because changing all the circuits to take this into account is, I think, a lot of unnecessary work if that can be fixed in the halo2 crate. We have to keep in mind that for production circuits with a large fixed paddings the performance penalty goes into dozens of minutes like > 30mins.
I can take a look at the halo2 code because to assess this task I don't have enough information about it yet.

@adria0 adria0 added the enhancement New feature or request label Jun 10, 2024
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

No branches or pull requests

4 participants