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

perf: optimize trace identifiers #5680

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Aug 20, 2023

Motivation

Identifying and decoding traces contributes the majority (usually >40%) of the time spent in running tests. We can avoid most of this by just using the existing check flags to skip identifying only when needed.

My assumptions were wrong. Generally, we decode traces on the main thread in parallel to the threads executing tests. This is a problem when there are a lot of them, and are sent batched (like for invariants). This causes the main thread to block for a while just decoding traces, which may also block after all tests are done.

This PR cleans up some tracing code, but the issue mentioned above will have to be fixed in another PR.

Solution

@DaniPopes DaniPopes changed the title fix: only identify traces if they should be included fix: only identify traces when needed Aug 20, 2023
@DaniPopes DaniPopes marked this pull request as ready for review August 20, 2023 17:42
@mattsse mattsse requested a review from Evalir August 20, 2023 18:10
Copy link
Member Author

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Best reviewed with "Hide whitespace"

Comment on lines -16 to +14
impl LocalTraceIdentifier {
pub fn new(known_contracts: &ContractsByArtifact) -> Self {
Self {
local_contracts: known_contracts
.iter()
.map(|(id, (abi, runtime_code))| (runtime_code.clone(), (id.clone(), abi.clone())))
.collect(),
}
impl<'a> LocalTraceIdentifier<'a> {
pub fn new(known_contracts: &'a ContractsByArtifact) -> Self {
Self { known_contracts }
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove useless re-allocations, we only need to be able to iterate over the 3 values (id, abi, code)

TraceKind::Setup => {
(verbosity == 4 && result.status.is_failure()) || verbosity >= 5
}
TraceKind::Deployment => false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Make this exhaustive and a bit clearer

Comment on lines 635 to 638
if should_include || gas_reporting {
decoder.identify(trace, &mut local_identifier);
decoder.identify(trace, &mut etherscan_identifier);
decoder.decode(trace).await;
Copy link
Member Author

@DaniPopes DaniPopes Aug 20, 2023

Choose a reason for hiding this comment

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

This is the main change, moved the identify calls from above the match to inside the if block

Copy link
Member Author

Choose a reason for hiding this comment

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

this change is really weird, it speeds up significantly when there are a lot of traces, but not when there are little? I don't really know or have the resources to investigate, so I'll remove it from this PR

@DaniPopes DaniPopes closed this Aug 20, 2023
@DaniPopes
Copy link
Member Author

Needs a fix, i would've made it a draft if i knew how to on mobile

@Evalir Evalir reopened this Aug 20, 2023
@Evalir Evalir changed the title fix: only identify traces when needed [DO NOT MERGE] fix: only identify traces when needed Aug 20, 2023
@Evalir
Copy link
Member

Evalir commented Aug 20, 2023

@DaniPopes gotcha, reopened and marked it as DO NOT MERGE—feel free to reclose if anything

@DaniPopes DaniPopes changed the title [DO NOT MERGE] fix: only identify traces when needed perf: only identify traces when needed Aug 21, 2023
@DaniPopes DaniPopes marked this pull request as draft August 21, 2023 12:06
@DaniPopes DaniPopes changed the title perf: only identify traces when needed perf: optimize trace identifiers Aug 21, 2023
@DaniPopes DaniPopes marked this pull request as ready for review August 21, 2023 14:29
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

love this. the changes here make sense

@@ -79,7 +79,7 @@ pub(crate) fn decode_cheatcode_inputs(
"serializeBytes32" |
"serializeString" |
"serializeBytes" => {
if verbosity == 5 {
if verbosity >= 5 {
Copy link
Member

Choose a reason for hiding this comment

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

nice catch!

@Evalir Evalir merged commit 369fb72 into foundry-rs:master Aug 21, 2023
15 checks passed
@DaniPopes DaniPopes deleted the opt-tracers branch August 21, 2023 15:29
mikelodder7 pushed a commit to LIT-Protocol/foundry that referenced this pull request Sep 12, 2023
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.

2 participants