-
Notifications
You must be signed in to change notification settings - Fork 347
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
Feat: Vm Profiling #1789
base: dev
Are you sure you want to change the base?
Feat: Vm Profiling #1789
Conversation
e04ad2b
to
5e1c1b9
Compare
27923bd
to
a1a732c
Compare
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.
overall looks pretty good, made some small comments about cleaning up docs
@yuwen01 the CLI CI test seems to be failing silently here.. Wdyt? |
9c5d7fc
to
40b61fc
Compare
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.
Left some doc comments, but logic is pretty clear now thanks to your comments. You can check that your changes to the book look good by running mdbook serve
from the sp1 root directory
b5a3684
to
6b834d1
Compare
@@ -56,69 +56,29 @@ fn main() { | |||
|
|||
This will log the cycle count for `block name` and include it in the `ExecutionReport` in the `cycle_tracker` map. | |||
|
|||
## Tracking Cycles with Tracing | |||
### Profiling the ZKVM |
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.
IMO this doc could be a bit clearer and friendlier. The title should be something like "Profiling a zkVM Program", and then it should explain what that means and give a sample firefox URL and maybe a screenshot. To explain how to use it, we should make it clear that you just need to set some env vars while ProverClient.execute is called, and then call a samply command. Then we can explain when you might want to adjust the sample rate (ex >100m cycles?).
It would also help to have a few lines about how to interpret the info in the profiler.
6ec9289
to
64417f6
Compare
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.
feature flagging profiling is WAY better than the debug assertions thing i like
in general, since everything we do is open source, we highly prioritize having clean code and docs. this pretty much involves
- complete sentences whenever possible. sometimes, when describing struct fields, it's okay to not have a complete sentence. something like this
// The profiler.
profiler: whatever
is fine, but you should still use caps and punctuation. Especially when writing docs, also try not to use run-on sentences -- claude can probably catch a lot of these grammar mistakes.
|
||
The `cycle-tracker` annotation is a convenient way to track cycles for specific sections of code. However, sometimes it can also be useful to track what functions are taking the most cycles across the entire program, without having to annotate every function individually. | ||
Profiling a zkvm program is a good way to get an understanding of what is bottlenecking your program, note only one program may be profiled at a time. |
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.
nit: ... program. Note only ...
|
||
First, we need to generate a trace file of the program counter at each cycle while the program is executing. This can be done by simply setting the `TRACE_FILE` environment variable with the path of the file you want to write the trace to. For example, you can run the following command in the `script` directory for any example program: | ||
To profile a program, you have to setup a script to execute the program, |
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.
nit: ... program. Many examples ...
|
||
Next, we can use the `cargo prove` CLI with the `trace` command to analyze the trace file and generate a table of instruction counts. This can be done with the following command: | ||
A larger sample rate will give you a smaller profile, it is the number of instructions in between each sample. |
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.
nit: ... profile. It is the ...
``` | ||
|
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.
nit: capitalize Firefox, program's execution.
@ctian1 thoughts on a screenshot?
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.
grammar mistake unsolved
crates/core/executor/src/profiler.rs
Outdated
let mut function_ranges = Vec::new(); | ||
let mut builder = ThreadBuilder::new(1, 0, std::time::Instant::now(), false, false); | ||
|
||
// We need to extract all the functions from the elf file |
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.
nit typo
crates/core/executor/src/profiler.rs
Outdated
let start_address = sym.st_value; | ||
let end_address = start_address + size - 4; | ||
|
||
// Now that we have the name lets immediately intern it so we only need to copy |
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.
nit: let's
crates/core/executor/src/profiler.rs
Outdated
Serde(#[from] serde_json::Error), | ||
} | ||
|
||
/// The ZKVM Profiler. |
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.
nit: use caps and punctuation in these doc comments
self.builder.add_sample( | ||
last_known_time, | ||
sample.stack.into_iter(), | ||
// We don't have a way to know the duration of each sample, so we just use 1us for |
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.
nit punc
crates/core/executor/src/executor.rs
Outdated
@@ -190,11 +192,52 @@ impl<'a> Executor<'a> { | |||
Self::with_context(program, opts, SP1Context::default()) | |||
} | |||
|
|||
/// Crete a new runtime for the program, and setup the profiler if `TRACE_FILE` env var is set |
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.
nit spelling
/// # Panics | ||
/// | ||
/// This function may panic if it fails to create the trace file if `TRACE_FILE` is set. | ||
/// Note: This function *will not* set up the profiler. | ||
#[must_use] |
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 clarify, the purpose of this function now is solely to maintain backwards compatibility?
846a2af
to
f4e042a
Compare
![An example screenshot of the Firefox Profiler](../profiling.png) | ||
|
||
#### Interpreting the Profile | ||
- The "time" measurement in the profiler is actually the number of cycles spent, |
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 is good, it would be nice to explain what each view in the profiler is and how to interpret it, like call tree (invert call stack), flame graph, the timeline at the top
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.
agreed
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.
like some people might not know how to read the flame graph, for example
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.
or they might not know to invert call stack in order to find the most frequent functions
Co-authored-by: Chris T. <[email protected]>
Co-authored-by: Chris T. <[email protected]>
f7c2fa1
to
bb14e17
Compare
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.
Some more comments on the docs
|
||
The full command to profile should look something like this |
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.
Maybe we should just provide an end-to-end example of how to profile the fibonacci example or something. Right now these docs are a bit redundant. For example, I don't really like the overview section at the top. If you explain stuff step by step in your main explanation, I think that will probably suffice
``` | ||
|
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.
grammar mistake unsolved
![An example screenshot of the Firefox Profiler](../profiling.png) | ||
|
||
#### Interpreting the Profile | ||
- The "time" measurement in the profiler is actually the number of cycles spent, |
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.
agreed
![An example screenshot of the Firefox Profiler](../profiling.png) | ||
|
||
#### Interpreting the Profile | ||
- The "time" measurement in the profiler is actually the number of cycles spent, |
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.
like some people might not know how to read the flame graph, for example
![An example screenshot of the Firefox Profiler](../profiling.png) | ||
|
||
#### Interpreting the Profile | ||
- The "time" measurement in the profiler is actually the number of cycles spent, |
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.
or they might not know to invert call stack in order to find the most frequent functions
Building on a previous PR #1713
The logic to preserve the callstack is now done during execution (debug only)
and by default the trace file is now a gecko profile json, which can opened via
samply load ...