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

feat: speedup test by only check constraints on less rows for MockProver#354

Merged
CPerezz merged 5 commits into
privacy-ethereum:mainfrom
scroll-tech:ci/speedup/rows
Mar 7, 2022
Merged

feat: speedup test by only check constraints on less rows for MockProver#354
CPerezz merged 5 commits into
privacy-ethereum:mainfrom
scroll-tech:ci/speedup/rows

Conversation

@lispc
Copy link
Copy Markdown
Collaborator

@lispc lispc commented Feb 26, 2022

Most of the rows in circuits are "muted" by q_step * ..... So for testing purpose, since we know which rows are "active" for our circuit, we can only test the constraints/lookups on "active" rows.

Speedup by this PR is significant, in my Mac M1, bitwise_gadget_simple test time changes from 640s to 47s. And in github CI, full CI time goes down from 42min to 12min, within which there are 7min compiling time. So the real speedup is about (42-7)/(12-7) = 7 times. If we implement rust compiling cache later, the CI time can be further reduce to 5min.

This PR is not ready for merge currently. The PR on halo2 side(privacy-ethereum/halo2#38) should be merged first, then I will change the halo2 in Cargo.toml here.

@github-actions github-actions Bot added crate-keccak Issues related to the keccak workspace member T-opcode Type: opcode-related and focused PR/Issue crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Feb 26, 2022
Comment thread zkevm-circuits/src/evm_circuit.rs Outdated
Comment thread zkevm-circuits/src/evm_circuit/execution.rs Outdated
Copy link
Copy Markdown
Contributor

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

Added a comment which can be related to modifications in the halo2 PR.

Let me know what you think :)

Comment thread Cargo.toml Outdated
Comment thread zkevm-circuits/src/evm_circuit.rs Outdated
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not submitting "Approve" yet mainly due to the halo2 dependency that must be updated once privacy-ethereum/halo2#38 is merged as CPerezz has mentioned.
I also think that the comments from z2trillion should be attended as well :)

Thanks a lot for working on this! We will all benefit from faster testing times in the CI :)

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Mar 6, 2022

I just merged privacy-ethereum/halo2#38 and then released a tag for it: https://github.com/appliedzkp/halo2/releases/tag/v2022_03_06

You can now update all Halo2 git deps for the workspace members and this can be merged after @z2trillion comments are addressed/resolved

cc @lispc

@github-actions github-actions Bot removed the T-opcode Type: opcode-related and focused PR/Issue label Mar 7, 2022
@lispc
Copy link
Copy Markdown
Collaborator Author

lispc commented Mar 7, 2022

CI error:

 Documenting bitvec v0.22.3
error: couldn't generate documentation: No such file or directory (os error 2)
Error:   |
  = note: failed to create or modify "/home/runner/work/zkevm-circuits/zkevm-circuits/target/doc/bitvec/vec/struct.BitVec.html"

error: could not document `bitvec`
Error: could not document `bitvec`

I think it is not related to this PR? maybe some upstream source ( cargo? bitvec? ) changed?

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Mar 7, 2022

CI error:

 Documenting bitvec v0.22.3
error: couldn't generate documentation: No such file or directory (os error 2)
Error:   |
  = note: failed to create or modify "/home/runner/work/zkevm-circuits/zkevm-circuits/target/doc/bitvec/vec/struct.BitVec.html"

error: could not document `bitvec`
Error: could not document `bitvec`

I think it is not related to this PR? maybe some upstream source ( cargo? bitvec? ) changed?

@CPerezz also encountered this error once, and if I remember correctly, it just went away by rerunning the CI. Currently we're depending on two different version of bitvec (one is pulled by halo2, the other is pulled by ethereum types). My impression is that the issue appears because both versions use the same path to build the documentation, and the documentations are built in parallel, which may cause a data race between the two versions reading and writing into the same folder.

From the cargo doc log:

Warning: The lib target `bitvec` in package `bitvec v0.22.3` has the same output filename as the lib target `bitvec` in package `bitvec v0.20.5 ([https://github.com/ed255/bitvec.git?rev=5cfc5fa8496c66872d21905e677120fc3e79693c#5cfc5fa8)`.](https://github.com/ed255/bitvec.git?rev=5cfc5fa8496c66872d21905e677120fc3e79693c#5cfc5fa8)%60.)

For now the easiest solution is re-running the CI when this happens. One of the versions of bitvec we're using is a patched fork controlled by us, maybe we can tweak it to use a different doc folder...

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Mar 7, 2022

Hi @ed255 @lispc .

As you can see, I re-run all checks and the error disappeared.
The issue I think, is related to a bug in Cargo which sometimes leads to a compilation error for the docs.

I'll file an issue for it later today, but for now don't worry too much. As this will disappear once the patch is not needed anymore.

@CPerezz
Copy link
Copy Markdown
Contributor

CPerezz commented Mar 7, 2022

I do also think @ed255 that if we use resolver=2 (since we're not using edition 2021 yet) we could get rid of this with a high probability.

Do you think making a PR for the resolver or the edition change would make sense?

Copy link
Copy Markdown
Contributor

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

LGTM!!

Nice job @lispc 🥳

Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-keccak Issues related to the keccak workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants