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

Feat: Vm Profiling #1789

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Conversation

nhtyy
Copy link
Collaborator

@nhtyy nhtyy commented Nov 14, 2024

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 ...

@nhtyy nhtyy force-pushed the n/profiling-in-executor branch 2 times, most recently from e04ad2b to 5e1c1b9 Compare November 14, 2024 00:52
Copy link
Contributor

@yuwen01 yuwen01 left a 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

book/developers/profiling.md Outdated Show resolved Hide resolved
crates/core/executor/src/profiler.rs Outdated Show resolved Hide resolved
crates/core/executor/src/profiler.rs Show resolved Hide resolved
crates/core/executor/src/profiler.rs Show resolved Hide resolved
@nhtyy
Copy link
Collaborator Author

nhtyy commented Nov 14, 2024

@yuwen01 the CLI CI test seems to be failing silently here.. Wdyt?

@nhtyy nhtyy force-pushed the n/profiling-in-executor branch 2 times, most recently from 9c5d7fc to 40b61fc Compare November 14, 2024 23:28
Copy link
Contributor

@yuwen01 yuwen01 left a 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

book/developers/profiling.md Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
crates/core/executor/src/profiler.rs Outdated Show resolved Hide resolved
crates/core/executor/src/profiler.rs Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
@nhtyy nhtyy force-pushed the n/profiling-in-executor branch 2 times, most recently from b5a3684 to 6b834d1 Compare November 16, 2024 00:24
@@ -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
Copy link
Member

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.

crates/core/executor/Cargo.toml Show resolved Hide resolved
crates/prover/src/lib.rs Outdated Show resolved Hide resolved
examples/fibonacci/trace.json Outdated Show resolved Hide resolved
examples/rsp/script/trace.json Outdated Show resolved Hide resolved
examples/rsp/trace.json Outdated Show resolved Hide resolved
@nhtyy nhtyy force-pushed the n/profiling-in-executor branch 7 times, most recently from 6ec9289 to 64417f6 Compare November 22, 2024 18:31
Copy link
Contributor

@yuwen01 yuwen01 left a 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

  1. 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.
Copy link
Contributor

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,
Copy link
Contributor

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.
Copy link
Contributor

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 ...

```

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar mistake unsolved

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit typo

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's

Serde(#[from] serde_json::Error),
}

/// The ZKVM Profiler.
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit punc

@@ -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
Copy link
Contributor

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]
Copy link
Contributor

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?

@nhtyy nhtyy force-pushed the n/profiling-in-executor branch 2 times, most recently from 846a2af to f4e042a Compare November 22, 2024 20:12
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
book/writing-programs/cycle-tracking.md Outdated Show resolved Hide resolved
![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,
Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

@yuwen01 yuwen01 left a 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
Copy link
Contributor

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

```

Copy link
Contributor

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,
Copy link
Contributor

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,
Copy link
Contributor

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,
Copy link
Contributor

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

@nhtyy nhtyy added the next-release A PR or an issue that should resolved before the next major release label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release A PR or an issue that should resolved before the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants