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

Add another Cache for accessing files in ArtifactBundles #1354

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Jan 31, 2024

When profiling (via symbolicator-stress) processing of javascript events, the majority of the time is spent extracting minified / sourcemap files from artifact bundles (which are essentially zip files).

We introduce yet another cache which can avoid this costly unzip process by keeping these unzipped files in memory.

For the above mentioned stresstest, this has lead to a throughput increase of 1000x. Though not surprisingly, the stresstest processes the exact same event over and over and has thus a 100% cache hit rate.

#skip-changelog

In a specific `stresstest` using a JS event that has a particularly large sourcemap, this leads to a speedup of up to 1_000x
@Swatinem Swatinem self-assigned this Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (60dec6f) 75.88% compared to head (b24da62) 75.99%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1354      +/-   ##
==========================================
+ Coverage   75.88%   75.99%   +0.10%     
==========================================
  Files          99      100       +1     
  Lines       14764    14846      +82     
==========================================
+ Hits        11204    11282      +78     
- Misses       3560     3564       +4     

Cargo.toml Show resolved Hide resolved
crates/symbolicator-js/src/lookup.rs Outdated Show resolved Hide resolved
crates/symbolicator-js/src/lookup.rs Outdated Show resolved Hide resolved
crates/symbolicator-js/src/service.rs Outdated Show resolved Hide resolved
@anonrig
Copy link
Contributor

anonrig commented Jan 31, 2024

@Swatinem Can you update the description to include any context about people who doesn't know anything about our talk/pair coding session? If you could include benchmark results, what are we doing to improve etc. it would be good for future documentation.

Copy link
Contributor

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

🚀

@Swatinem Swatinem enabled auto-merge (squash) January 31, 2024 15:30
@Swatinem Swatinem merged commit 8218752 into master Jan 31, 2024
15 checks passed
@Swatinem Swatinem deleted the swatinem/cache-bundle-lookup branch January 31, 2024 15:31
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.

3 participants