Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Perf improvements for keccak,state circuit #1253

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

pinkiebell
Copy link
Contributor

Description

  • Improves the keccak performance for padding rows significantly
  • small performance gain in state circuit with high padding rows

- Improves the keccak performance for padding rows significantly
- small performance gain in state circuit with high padding rows
@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Feb 24, 2023
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

That makes complete sense! Indeed, I think we should have a logging feature directly probably. So that we avoid situations like this one..

WDYT @ed255 ??

Thanks for this @pinkiebell good catch!

@CPerezz CPerezz added this pull request to the merge queue Feb 27, 2023
Merged via the queue into privacy-scaling-explorations:main with commit 7dc1b38 Feb 27, 2023
@pinkiebell
Copy link
Contributor Author

The greatest perf improvement came from the precomputed padding_rows. That alone improved perf about 6.7 times.
After some more digging I also see that the closure for assign_region is invoked twice with the simple floor planner. I might sense to keep that in mind to move heavy computation outside that closure and just reference it when doing the assignments.

@pinkiebell pinkiebell deleted the 2023-02-24 branch February 28, 2023 05:54
@CPerezz
Copy link
Member

CPerezz commented Feb 28, 2023

@pinkiebell maybe makes sense to file an issue in Halo2 for that? So that we can check it deeper?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants