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

Update shared hashmap to use RwLock instead of threadlocal #213

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

mattcompiles
Copy link
Contributor

Motivation

While doing some performance testing I found that the threadlocal version of the resolver cache was have a large negative impact on build times. Reverting back to the RwLock version is almost halving the asset graph build times in native builds. I have also validated that V2 builds are also performing better with this change, however not by the same margin.

Changes

  • Migrate back to RwLock Hash maps for the resolver cache

Checklist

  • Existing or new tests cover this change

Copy link
Contributor

@yamadapc yamadapc left a comment

Choose a reason for hiding this comment

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

This doesn't revert the entire PR that moved to thread-local ; I'd rather revert everything or at least understand where this makes sense vs. not at the moment.

I have also validated that V2 builds are also performing better with this change

How much better are V2 builds? It's weird because this did not cause a regression when rolled-out on our metrics.

It could be that this strategy works for the caches you didn't revert but not the packages/tsconfigs case or that it works for none & something is wrong with benchmarks/measurements (for example the CI agents we're using have a lot more memory than your computer).

@mattcompiles
Copy link
Contributor Author

This doesn't revert the entire PR that moved to thread-local ; I'd rather revert everything or at least understand where this makes sense vs. not at the moment.

I did originally go to revert but the revert commit needed changes anyway so I decided to just do this file as it was what I had experimented with.

How much better are V2 builds? It's weird because this did not cause a regression when rolled-out on our metrics.

I only did one test just to validate it wasn't a regression for V2 and I built the asset graph about 20s quicker, 4:48 for with thread local vs 4:25 with this change.

@mattcompiles mattcompiles changed the title Revert threadlocal cache Update shared hashmap to use RwLock instead of threadlocal Nov 14, 2024
@mattcompiles mattcompiles merged commit c9cb92c into main Nov 14, 2024
17 checks passed
@mattcompiles mattcompiles deleted the matt/back-to-locked-hashmap branch November 14, 2024 23:43
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