-
Notifications
You must be signed in to change notification settings - Fork 199
Add Cairo Runner Builder #2223
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
base: 2.x.y
Are you sure you want to change the base?
Add Cairo Runner Builder #2223
Conversation
Benchmark Results for unmodified programs 🚀
|
aec3eb4
to
a93adb9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x.y #2223 +/- ##
==========================================
- Coverage 96.66% 95.65% -1.02%
==========================================
Files 103 103
Lines 43646 44115 +469
==========================================
+ Hits 42191 42198 +7
- Misses 1455 1917 +462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
To maintain behaviour backwards compatibility
1cb0d0f
to
513bf4b
Compare
071f873
to
8df8954
Compare
ModBuiltinRunner::new_mul_mod(&ModInstanceDef::new(Some(1), 1, 96), true).into() | ||
} | ||
}; | ||
self.builtin_runners.push(builtin_runner); |
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.
Could this lead to a builtin_runners with the same builtin runner multiple times? If so, isn't that a problem? Also this lets the user to add runners that are not part of the layout. Is that a problem?
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.
Could this lead to a builtin_runners with the same builtin runner multiple times?
Yes, if the user specifies multiple times the same builtin. Receiving the builtin runners is required as the user may only want to initialize the builtins required by the entrypoint that is going to be executed. This is a problem in the current Cairo Runner API also.
If so, isn't that a problem?
I am not sure if its a problem, but it never came up.
Also this lets the user to add runners that are not part of the layout. Is that a problem?
The current behavior ignores the layout, so I don't think its a problem, but I am not sure really.
vm/src/vm/runners/cairo_runner.rs
Outdated
/// Depends on: | ||
/// - [initialize_base_segments](Self::initialize_base_segments) | ||
/// - [initialize_builtin_runners_for_layout](Self::initialize_builtin_runners_for_layout) |
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.
This means that both of those methods have to be used before this one? If so, why not unifying them into one single method?
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.
There are at least 2 ways of initializing the builtin runners (there may be more):
- Initializing the entrypoint builtin runners (Cairo 1 - sequencer).
- Initializing the layout builtin runners, according to the program builtins (Cairo 0 - sequencer).
The flow, at least for the use cases that I researched, seems to be like this:
initialize_base_segments
.- Either
initialize_builtin_runners_for_layout
orinitialize_builtin_runners
. Now that I think of it, the code comment is wrong. initialize_builtin_segments
.
There are two reasons why I didn't abstract it yet into higer-level functions:
- I didn't want to create abstractions prematurely, as there are some usecases that I am not considering yet.
- I didn't want to depart much from the current Cairo Runner building API.
I totally agree in that we need to find a better/safer API, but I am not sure what is the best way to do it yet.
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.
I think it's fine to let the user decide how to build the runner, at least with the same flexibility as the previous one. It is true that it may be quite unsafe, as calling functions in a different order might result in an inconsistent state. However, we've seen some use cases where the runner needs to be built in exotic ways. So I think that, in our way of creating a safer api, we you consider these things as they shouldn't be change.
Co-authored-by: DiegoC <[email protected]>
bcee2c1
to
84d3e77
Compare
Add Cairo Runner Builder
The
CairoRunner
has two main responsibilities:This PR introduces a new structure
CairoRunnerBuilder
that handles only the initialization of the Cairo VM. Doing so not only improves the API, but also allows for some optimizations that were not possible before.Optimizations
Each time we call the same contract, we do the same initialization logic:
This could be cached. The new builder implementes
Clone
, so we can perform the initialization logic only once, and reuse it for all the calls to that contract.More over, when compiling hints, we are cloning the program's hints in order to put it into an
Rc
, which is required by thecompile_hint
function. To fix this, we are actually taking the program's hints (mem::take
), instead of cloning it. This is safe, as long as the program's hints are not accessed after compiling them (which is not the case). Note that this optimization was possible even without the new builder.Alternatives
The whole idea of this new abstraction is that we allow a way for "cloning" the state of a runner, prior to the execution. I considered other approaches before settling for this one:
The current builder has a method
fn build(self)
that consumes itself, and builds a CairoRunner. We could, instead, have something closer to a "factory" with a methodfn build(&self)
. This implies that we wouldn't need to clone something, just callbuild
multiple times. The downside of this is that if the user doesn't want to cache the builder, and just build the runner once, it would be forced to clone the internal fields. The flow would look like this:Allow a way for Cloning the CairoRunner directly. We cannot derive Clone for the CairoRunner, so we need to have a manual implementation that skips some of fields. This would work, but implies cluttering the CairoRunner with even more initialization logic. Having a dedicated structure for this is a step in the right direction of improving the API. The flow would look like this:
Similar to the last alternative, we could do the whole initialization in the CairoRunner, and provide a way for building an alternative struct
CairoRunnerSnapshot
. This struct should be clonable, and should be able to be used to build aCairoRunner
again. The flow would look like this:Benchmarks
I did some benchmarks over block ranges:
Some caveats:
Sequencer Code
The required code for the benchmark can be found here: https://github.com/lambdaclass/sequencer/compare/main-v0.14.0..main-v0.14.0-builder
Cairo 0 code
In the sequencer, I added the following code:
After retrieving the builder, we call:
After execution, we set the instruction cache for the following executions:
Cairo 1 Code
For Cairo 1, I did something similar:
In the sequencer, I added the following code:
After retrieving the builder, we call:
After execution, we set the instruction cache for the following executions:
Checklist