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

Witgen: Remove sequence cache for block machines #1560

Closed
wants to merge 1 commit into from

Conversation

georgwiese
Copy link
Collaborator

Fixes #1559

The sequence cache is often causing trouble, and I wonder how big the performance benefit even is after #1528 (and given that we don't spend that much time in secondary machines).

We should benchmark this PR and see what the impact is. If it is small, we can through away most of executor/src/witgen/sequence_iterator.rs!

@georgwiese
Copy link
Collaborator Author

On the Keccak RISC-V example, the block machines do seem to be roughly 2x slower. These are two representative runs:

========== BEFORE
 == Witgen profile (1766802 events)
   39.6% (    4.1s): FixedLookup
   37.7% (    3.9s): Main Machine
   12.7% (    1.3s): Secondary machine 0: main_binary (BlockMachine)
    6.3% ( 652.5ms): witgen (outer code)
    3.1% ( 319.1ms): Secondary machine 2: main_shift (BlockMachine)
    0.6% (  57.7ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 251.6µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 10.349325541s

========== AFTER
 == Witgen profile (3086124 events)
   35.4% (    4.6s): FixedLookup
   29.8% (    3.8s): Main Machine
   23.3% (    3.0s): Secondary machine 0: main_binary (BlockMachine)
    5.7% ( 738.2ms): Secondary machine 2: main_shift (BlockMachine)
    5.3% ( 677.5ms): witgen (outer code)
    0.4% (  52.3ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 335.5µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 12.854500542s

@georgwiese
Copy link
Collaborator Author

#1562 is a better solution.

@georgwiese georgwiese closed this Jul 11, 2024
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.

Witgen produces wrong witness when reordering constraints
1 participant