Support for Flamegraph

[zemse]

Motivation

This PR closes #776. Old PR #8315.

Solution

Adds a flamegraph and flamechart flag to the test subcommand. Using --match-test and/or --match-contract just a single test case must be run.

Example:

# sorts functions by name and merges them
forge test --match-test test_hash_var_len_2 --flamegraph

# does not merge functions and prints them in execution order, basically visualizes the trace chart
forge test --match-test test_hash_var_len_2 --flamechart 

This will execute just the test_hash_1_a_sol test and generate a flamegraph.svg file in cache dir and will also try to open this file in the default program.

[klkvr]

Thank you for this! Left several comments

Wondering if --flamegraph should automatically enable --decode-internal? I think it doesn't hurt but would make flamegraphs more informative by default

It seems that current folded stack trace impl is a bit incorrect. Not yet sure what's the issue, but for me the flamegraph for the following test does not look correct:

contract Contract {
    uint256 number;

    function run() public {
        for (uint i = 0; i < 10; i++) {
            number += 1;
        }
    }
}

contract TestContract {
    function test() public {
        Contract c = new Contract();
        c.run();
    }
}

[zemse]

Sorry for the delay. Thanks for the review!

I have made the changes and rebased the branch. Please re-review.

[klkvr]

lgtm, tested locally and flamegraphs are looking good, UX is smooth too!

let's merge FoldedStackTraceBuilder into signle file as it's not that large, and would also appreciate some docs on what it's doing (especially on the struct contents and how those are manipulated)

[zemse]

Thanks for checking it out! I've resolved the review comments, added some comments, and rebased the branch.

[klkvr]

overall lgtm, have a question re merging of entires:

let mut trace = super::FoldedStackTraceBuilder::default();
trace.enter("top".to_string(), 500);
trace.enter("child_a".to_string(), 100);
trace.exit();
trace.enter("child_a".to_string(), 200);
trace.exit();

assert_eq!(
    trace.build_without_subtraction(),
    vec![
        "top 500", //
        "top;child_a 100",
        "top;child_a 200",
    ]
);

shouldn't child_a entries end up merged here?

[iFrostizz]

Always found that the --debug <TEST_NAME> was a bit broken and conflicting with the current way of selecting tests.
We already have --matching-test so adding a --flamegraph <TEST_NAME> adds some new handling while you could just return an error if the matching failed to select only 1 test. What we may want though is a way to select a single test efficiently, without having to combine the --matching-contract and --matching-test in the case of duplicated contract name. Cargo does it well because it has namespaces by default, but could we consider having something like

forge test invariants::VaultTest::testDepositTokens --flamegraph

This way, the options to select tests would not be mutually exclusive.

[iFrostizz]

Congrats on this btw!!!

[zemse]

shouldn't child_a entries end up merged here?

@klkvr Currently, I was generating a flamechart instead. When it's flamegraph then the inferno lib sorts the lines to render with merged functions.

@iFrostizz thanks for feedback, it makes sense to use general tests matching logic.

I have changed the --flamegraph to be a boolean flag and use tests using the general test matching API. Also added --flamechart which is a variation. So the API looks like

# sorts functions by name and merges them
forge test --match-test test_hash_var_len_2 --flamegraph

# does not merge functions and prints them in execution order, basically visualizes the trace chart
forge test --match-test test_hash_var_len_2 --flamechart 

Please try and would love feedback!

[klkvr]

LGTM, works great for me locally!

left a couple small comments, should be quick fixes

[klkvr]

Note: profiling gas is a bit different vs traditional stack sampling, so there are some bugs I'm observing rn which we can look into in follow-ups. Mostly its related to refunds which result in negative cost code paths or children spending more gas than parents

[mattsse]

ty!

pending dep nits

[gakonst]

I cloned the PR and ran it against Solady, but something seems to not be matching? https://github.com/Vectorized/solady

 /Users/georgios/.foundry/foundry-rs/foundry/target/debug/forge t --flamegraph testSafeTransferFromToERC721RecipientWithData
No tests match the provided pattern:
        match-path: `testSafeTransferFromToERC721RecipientWithData`
Error:
No tests to run

when there's clearly such a test

 rg testSafeTransferFromToERC721RecipientWithData
test/ERC721.t.sol
776:    function testSafeTransferFromToERC721RecipientWithData(uint256 id, bytes memory data) public {

which the normal matcher hits

forge t --match-test testSafeTransferFromToERC721RecipientWithData
[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for test/ERC721.t.sol:ERC721Test
[PASS] testSafeTransferFromToERC721RecipientWithData(uint256,bytes) (runs: 256, μ: 524351, ~: 515913)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 37.57ms (36.84ms CPU time)

[klkvr]

@gakonst this should be forge t --flamegraph --mt testSafeTransferFromToERC721RecipientWithData

--flamegraph does not accept test function arg, thus function name was parsed as cli arg (equivalent to --match-path)

[zemse]

I've updated the PR description to reflect the current code. Sorry for the inconvenience.

addressed