Skip to content

Commit

Permalink
Block machine witgen: Always run default sequence after the cached se…
Browse files Browse the repository at this point in the history 
…quence (#1562)

Fixes #1559 (alternative to #1560)

With this PR, we always run the "default" sequence iterator, even if we
have a cached sequence (which is still run before). This way, if the
cached sequence was not sufficient to solve the entire block, the
default solving sequence will have another attempt.

The reason this doesn't lead to a dramatic performance degradation is
because since #1528, we skip identities that have been completed. In the
typical case, the cached sequence will have completed most identities.

The reason this is better than #1560 is because the cached sequence
typically makes progress with every identity, whereas the default
iterator does not.

## Benchmark

I ran the RISC-V Keccak example 3 times. It looks like the time spent in
block machines increases by roughly 20%. Given that this fixes a bug and
the overall time spent in block machines is small (even in the
bitwise-heavy Keccak example), I think it's worth it!

### Main

```
 == Witgen profile (1766802 events)
   44.5% (    4.4s): FixedLookup
   36.3% (    3.6s): Main Machine
    9.7% ( 964.7ms): Secondary machine 0: main_binary (BlockMachine)
    6.8% ( 669.3ms): witgen (outer code)
    2.4% ( 236.0ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  32.6ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 183.7µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.907174375s

 == Witgen profile (1766802 events)
   41.0% (    3.8s): FixedLookup
   39.0% (    3.6s): Main Machine
   10.2% ( 951.7ms): Secondary machine 0: main_binary (BlockMachine)
    6.9% ( 644.2ms): witgen (outer code)
    2.5% ( 231.5ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  32.1ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 183.4µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.295457333s

 == Witgen profile (1766802 events)
   43.7% (    4.2s): FixedLookup
   37.0% (    3.6s): Main Machine
   10.0% ( 963.6ms): Secondary machine 0: main_binary (BlockMachine)
    6.6% ( 636.3ms): witgen (outer code)
    2.4% ( 234.7ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  29.0ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 190.8µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.677017958s
```

### This branch

```
 == Witgen profile (1986686 events)
   43.3% (    4.3s): FixedLookup
   36.2% (    3.6s): Main Machine
   11.5% (    1.1s): Secondary machine 0: main_binary (BlockMachine)
    6.0% ( 600.2ms): witgen (outer code)
    2.7% ( 273.3ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  28.6ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 203.4µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.975125084s

 == Witgen profile (1986686 events)
   40.4% (    3.9s): FixedLookup
   37.2% (    3.6s): Main Machine
   12.1% (    1.2s): Secondary machine 0: main_binary (BlockMachine)
    7.1% ( 687.7ms): witgen (outer code)
    2.9% ( 276.7ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  30.3ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 197.3µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.619824375s

 == Witgen profile (1986686 events)
   42.4% (    4.2s): FixedLookup
   36.1% (    3.6s): Main Machine
   11.9% (    1.2s): Secondary machine 0: main_binary (BlockMachine)
    6.6% ( 654.9ms): witgen (outer code)
    2.8% ( 276.6ms): Secondary machine 2: main_shift (BlockMachine)
    0.3% (  29.1ms): Secondary machine 1: main_memory (DoubleSortedWitnesses)
    0.0% ( 202.3µs): Secondary machine 3: main_split_gl (BlockMachine)
  ---------------------------
    ==> Total: 9.957052209s
```
  • Loading branch information
@georgwiese
georgwiese committed Jul 15, 2024
1 parent 97aea6d commit ff8f81e
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 32 deletions.
19 changes: 0 additions & 19 deletions executor/src/witgen/machines/block_machine.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ impl<'a, T: FieldElement> ProcessResult<'a, T> {
false => ProcessResult::Incomplete(updates),
}
}

fn is_success(&self) -> bool {
match self {
ProcessResult::Success(_, _) => true,
ProcessResult::Incomplete(_) => false,
}
}
}

fn collect_fixed_cols<T: FieldElement>(
Expand Down Expand Up @@ -517,18 +510,6 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {
let process_result =
self.process(mutable_state, &mut sequence_iterator, outer_query.clone())?;

let process_result = if sequence_iterator.is_cached() && !process_result.is_success() {
log::debug!("The cached sequence did not complete the block machine. \
This can happen if the machine's execution steps depend on the input or constant values. \
We'll try again with the default sequence.");
let mut sequence_iterator = self
.processing_sequence_cache
.get_default_sequence_iterator();
self.process(mutable_state, &mut sequence_iterator, outer_query.clone())?
} else {
process_result
};

match process_result {
ProcessResult::Success(new_block, updates) => {
log::trace!(
Expand Down
35 changes: 22 additions & 13 deletions executor/src/witgen/sequence_iterator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ pub enum ProcessingSequenceIterator {
/// The default strategy
Default(DefaultSequenceIterator),
/// The machine has been run successfully before and the sequence is cached.
Cached(<Vec<SequenceStep> as IntoIterator>::IntoIter),
Cached(
<Vec<SequenceStep> as IntoIterator>::IntoIter,
DefaultSequenceIterator,
),
/// The machine has been run before, but did not succeed. There is no point in trying again.
Incomplete,
}
Expand All @@ -179,24 +182,17 @@ impl ProcessingSequenceIterator {
pub fn report_progress(&mut self, progress_in_last_step: bool) {
match self {
Self::Default(it) => it.report_progress(progress_in_last_step),
Self::Cached(_) => {} // Progress is ignored
Self::Cached(_, _) => {} // Progress is ignored
Self::Incomplete => unreachable!(),
}
}

pub fn has_steps(&self) -> bool {
match self {
Self::Default(_) | Self::Cached(_) => true,
Self::Default(_) | Self::Cached(_, _) => true,
Self::Incomplete => false,
}
}

pub fn is_cached(&self) -> bool {
match self {
Self::Default(_) => false,
Self::Cached(_) | Self::Incomplete => true,
}
}
}

impl Iterator for ProcessingSequenceIterator {
Expand All @@ -205,7 +201,13 @@ impl Iterator for ProcessingSequenceIterator {
fn next(&mut self) -> Option<Self::Item> {
match self {
Self::Default(it) => it.next(),
Self::Cached(it) => it.next(),
// After the cached iterator is exhausted, run the default iterator again.
// This is because the order in which the identities should be processed *might*
// depend on the concrete input values.
// In the typical scenario, most identities will be completed at this point and
// the block processor will skip them. But if an identity was not completed before,
// it will try again.
Self::Cached(it, default_iterator) => it.next().or_else(|| default_iterator.next()),
Self::Incomplete => unreachable!(),
}
}
Expand Down Expand Up @@ -246,7 +248,14 @@ impl ProcessingSequenceCache {
match self.cache.get(&left.into()) {
Some(CacheEntry::Complete(cached_sequence)) => {
log::trace!("Using cached sequence");
ProcessingSequenceIterator::Cached(cached_sequence.clone().into_iter())
ProcessingSequenceIterator::Cached(
cached_sequence.clone().into_iter(),
DefaultSequenceIterator::new(
self.block_size,
self.identities_count,
Some(self.outer_query_row as i64),
),
)
}
Some(CacheEntry::Incomplete) => ProcessingSequenceIterator::Incomplete,
None => {
Expand Down Expand Up @@ -291,7 +300,7 @@ impl ProcessingSequenceCache {
.is_none());
}
ProcessingSequenceIterator::Incomplete => unreachable!(),
ProcessingSequenceIterator::Cached(_) => {} // Already cached, do nothing
ProcessingSequenceIterator::Cached(_, _) => {} // Already cached, do nothing
}
}
}

0 comments on commit ff8f81e

Please sign in to comment.