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

Improve performance of Cache::analyze #2051

Merged
merged 11 commits into from
Mar 14, 2024
Merged

Improve performance of Cache::analyze #2051

merged 11 commits into from
Mar 14, 2024

Conversation

aumetra
Copy link
Member

@aumetra aumetra commented Mar 12, 2024

Improve the performance of the Cache::analyze function.
When analyzing the current benchmarks using perf, you'll see that most of the time is spent inside the FuncValidator::valdiate function.

image

This PR changes the logic to only validate the function bodies when the WASM is saved into the cache since we already assume that the cache only contains modules which check all the validation boxes.

Now the largest timespan spent is on the SHA-256 checksum generation.

image

Another minor change is that the function body validation is now done in parallel which will mostly benefit WASM modules with a lot of functions.
The downside of this is that the allocations can't be reused since that would imply shared mutable ownership across threads, and using a mutex here would just make this code essentially sequential again.

But there is still an ~15% performance gain over not doing it in parallel on average in my non-reliable benchmarks (they were performed on my workstation with rust-analyzer, Firefox, etc. open).

Closes #2033

@aumetra aumetra requested a review from webmaster128 March 12, 2024 14:32
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

🙌

@webmaster128
Copy link
Member

Now the largest timespan spent is on the SHA-256 checksum generation.

That's surprising to me. Loading the file from disk is much faster than hashing it? Strange. But we probably talk about very small values at this point anyways.

@aumetra aumetra force-pushed the aw/analyze-performance branch from cd19735 to 8b1f750 Compare March 12, 2024 16:39
@aumetra aumetra requested a review from webmaster128 March 14, 2024 14:22
@aumetra
Copy link
Member Author

aumetra commented Mar 14, 2024

I am not quite sure how this change could even have impacted the cosmwasm_check unittests? They seem unrelated on a first glance?

@webmaster128
Copy link
Member

I am not quite sure how this change could even have impacted the cosmwasm_check unittests? They seem unrelated on a first glance?

I think what is happening here is that the corrupted.wasm now returns a different error because the order of checks in check_wasm changed. I think we can just update the test.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Very nice. Could you add a CHANGELOG.md entry to the Unreleased section, that briefly explains what we did here and links the PR?

@aumetra aumetra requested a review from webmaster128 March 14, 2024 16:02
@aumetra
Copy link
Member Author

aumetra commented Mar 14, 2024

Okay, pushed on wrong branch. One sec.

@aumetra aumetra merged commit 56a5946 into main Mar 14, 2024
29 checks passed
@aumetra aumetra deleted the aw/analyze-performance branch March 14, 2024 16:16
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.

Benchmark and optimize analyze call
3 participants