-
Notifications
You must be signed in to change notification settings - Fork 251
FastProcessor now returns an error when cycle count is exceeded
#2537
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
Conversation
710a357 to
6e19dd6
Compare
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
3b1f781 to
f3e2754
Compare
| /// Creates a new `FastProcessor` instance, set to debug mode, with the given stack | ||
| /// and advice inputs. | ||
| /// Creates a new `FastProcessor` instance with the given stack and advice inputs, where | ||
| /// debugging and tracing are enabled. | ||
| /// | ||
| /// # Panics | ||
| /// - Panics if the length of `stack_inputs` is greater than `MIN_STACK_DEPTH`. | ||
| pub fn new_debug(stack_inputs: &[Felt], advice_inputs: AdviceInputs) -> Self { | ||
| Self::initialize(stack_inputs, advice_inputs, true) | ||
| Self::new_with_options( | ||
| stack_inputs, | ||
| advice_inputs, | ||
| ExecutionOptions::default().with_debugging(true).with_tracing(), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I enabled tracing with the new_debug() to preserve former functionality (mostly in tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but would be great to "normalize" ExecutionOptions at some point. For example, it is a bit odd that with_dubbing() takes a parameter while with_tracing() does not.
f3e2754 to
cbdabec
Compare
15bd24a to
bcf0479
Compare
| /// Returns true if decorators should be executed. | ||
| /// | ||
| /// This corresponds to either being in debug mode (for debug decorators) or having tracing | ||
| /// enabled (for trace decorators). | ||
| fn should_execute_decorators(&self) -> bool { | ||
| self.in_debug_mode() || self.options.enable_tracing() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Al-Kindi-0 I re-requested a review, since I added this method since you approved (which I only noticed was needed when working on #2539). Basically, we should execute decorators not only in debug mode, but also when tracing is enabled.
This has the unfortunate consequence that the processor will be slowed down when only tracing (but not debug mode) is enabled.
This is a by-product of the fact that AsmOp decorators should not be lumped in with Debug and Trace decorators, since they do not need to be executed at runtime. Currently, if tracing mode is enabled (or even debug mode just for debug decorators), the processor needlessly loops through all AsmOp decorators, which really isn't needed - we only need to look them up when an error occurs. AsmOp decorators should be stored separately in their own CSR matrix. This is as previously discussed with @huitseeker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #2541 to track
bobbinth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left just a few small comments inline.
Also, now we are doing a bit more work on every cycle - did this have any noticeable impact on performance?
| pub fn new_with_options( | ||
| stack_inputs: &[Felt], | ||
| advice_inputs: AdviceInputs, | ||
| options: ExecutionOptions, | ||
| ) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but we could consider encapsulating stack and advice inputs into a single struct - e.g., something like ProgramInputs.
| /// Creates a new `FastProcessor` instance, set to debug mode, with the given stack | ||
| /// and advice inputs. | ||
| /// Creates a new `FastProcessor` instance with the given stack and advice inputs, where | ||
| /// debugging and tracing are enabled. | ||
| /// | ||
| /// # Panics | ||
| /// - Panics if the length of `stack_inputs` is greater than `MIN_STACK_DEPTH`. | ||
| pub fn new_debug(stack_inputs: &[Felt], advice_inputs: AdviceInputs) -> Self { | ||
| Self::initialize(stack_inputs, advice_inputs, true) | ||
| Self::new_with_options( | ||
| stack_inputs, | ||
| advice_inputs, | ||
| ExecutionOptions::default().with_debugging(true).with_tracing(), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but would be great to "normalize" ExecutionOptions at some point. For example, it is a bit odd that with_dubbing() takes a parameter while with_tracing() does not.
| fn should_execute_decorators(&self) -> bool { | ||
| self.in_debug_mode() || self.options.enable_tracing() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add #[inline(always)] here as well?
Also, not for this PR, but I'm wondering if we should simplify the debug mode - basically, have a single setting that enables both debugging and tracing. I'm not really sure there is a lot of benefit of having them separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add #[inline(always)] here as well?
Done
Also, not for this PR, but I'm wondering if we should simplify the debug mode - basically, have a single setting that enables both debugging and tracing. I'm not really sure there is a lot of benefit of having them separate.
Agreed, though let's address this when we know whether better what tracing ends up looking like.
bcf0479 to
2bf0d59
Compare
2bf0d59 to
7685950
Compare
|
|
||
| if stopper.should_stop(self) { | ||
| ControlFlow::Break(()) | ||
| if self.clk >= self.options.max_cycles() as usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer @bobbinth's question about performance impact: I ran make exec-info and a blake3 proving benchmark on both this branch and next.
Results (blake3_1to1 prove):
- PR branch: total 33.1s,
execute_for_trace7.45ms,build_trace78.3ms - next branch: total 33.1s,
execute_for_trace11.1ms,build_trace87.0ms
No regression, execution is actually slightly faster on this branch (likely noise, but definitely not slower).
…Two` This is not an actual requirement, and some tests make use of fragments of size that are not a power of 2.
… use it internally
We also add a new method `FastProcessor::should_execute_decorator()`, which takes into account that we should execute decorators in debug mode *and* when tracing is enabled.
…)` method. Continuations should be passed directly rather than "fixing the continuation" after the call to `increment_clk()` with `map_break()` as was done previously. This is generally cleaner, and allows the `increment_clk_*()` methods to return internal errors as well.
This is done in the new `FastProcessor::increment_clk_with_continuation()`.
7685950 to
231ef56
Compare
Closes #2535
As part of this change, we also added a new constructor
FastProcessor::new_with_options(), which allows users to pass in aExecutionOptions.