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

[WIP] Support for Flamegraph #8315

Closed

Conversation

zemse
Copy link

@zemse zemse commented Jun 30, 2024

Motivation

This PR closes #776 and builds on top of #8222.

Solution

The current implementation modifies existing logic in test subcommand and evm/traces crate.

It adds a flamegraph flag to the test subcommand. The interface is similar to the debug flag. A pattern is to be provided to the flamegraph flag and additional match-contract or match-path maybe used. Finally it is required that just a single test case must be run.

Example:

forge test --flamegraph test_hash_1_a_sol --decode-internal

This will execute the test_hash_1_a_sol test and generate a flamegraph.svg file in the project root.

Copy link
Author

@zemse zemse left a comment

Choose a reason for hiding this comment

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

Just some things I'd like to highlight

arena: &CallTraceArena,
decoder: &CallTraceDecoder,
) -> Result<String, std::fmt::Error> {
) -> Result<(String, Vec<String>), std::fmt::Error> {
Copy link
Author

@zemse zemse Jul 2, 2024

Choose a reason for hiding this comment

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

I am adding the logic in this module itself to prevent code duplication, since logic for the folded stack trace is minimal here. Maybe the function render_trace_arena be renamed or some refactoring to be done to have this logic be used for both purposes: generating console printable traces and folded stack traces for flamegraph.

let mut folded_stack_lines = render_trace_arena(arena, decoder).await?.1;
folded_stack_lines.reverse();

let file_name = "flamegraph.svg";
Copy link
Author

Choose a reason for hiding this comment

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

This is placing the svg in the project root. This can be moved to a subdirectory / filename. Any preferences?

Copy link
Member

@klkvr klkvr Jul 4, 2024

Choose a reason for hiding this comment

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

Perhaps we can open it right away? flamegraph-rs is using opener in their CLI interface:

https://github.com/flamegraph-rs/flamegraph/blob/50611df2c9ee541c53689dda3d5164019cd778e8/src/lib.rs#L459
https://crates.io/crates/opener

let writer = std::fs::File::create(file_name).unwrap();

let mut options = inferno::flamegraph::Options::default();
options.flame_chart = true;
Copy link
Author

Choose a reason for hiding this comment

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

The inferno crate provides some options. Some of them could be passed on to user like choosing between flamegraph and flamechart.

Just realised that adding these flags on the test subcommand is not a good idea. Separate subcommand like forge flamegraph makes more sense? (I did this in test subcommand to prevent code duplication)

@klkvr
Copy link
Member

klkvr commented Jul 4, 2024

hey @zemse mind setting base branch to #8222? should be easier to review after that

also trace printing logic will change after #8224 and render_trace_arena wouldn't have such detailed access to internal fns and traces, so I think it makes sense to move folded traces construction to separate fn. It should be similar to how render_trace_arena looks now, just without any redering logic

@zemse zemse changed the base branch from master to klkvr/internal-fns-in-traces July 5, 2024 20:50
@DaniPopes DaniPopes deleted the branch foundry-rs:klkvr/internal-fns-in-traces July 11, 2024 16:20
@DaniPopes DaniPopes closed this Jul 11, 2024
@klkvr
Copy link
Member

klkvr commented Jul 11, 2024

@zemse PR got automatically closed after #8222 was merged, could you please open another one, building on master this time?

@DaniPopes
Copy link
Member

oh huh, I guess because #8222's branch was deleted, this was too. Didn't know that's how it worked, sorry about that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants