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

chore: drop slotmap and simplify locking #4696

Merged
merged 3 commits into from
Dec 5, 2024
Merged

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Dec 5, 2024

Summary

This PR drops the slotmap dependency in favor of papaya, and simplifies some of the locking inside the workspace settings.

The main tradeoff this makes is that we need to perform more cloning of the Settings struct, which is a rather large one. To mitigate the impact of this I changed Matcher such that is holds an Arc internally, making it cheap to clone. As part of this, I was also able to get rid of Matcher's internal RwLock by utilizing papaya again.

Test Plan

CI should remain green.

Also checked our release build against the unleash repository again. These are performance numbers I'm seeing on that repo (I re-ran each a couple of times to make sure these numbers aren't heavy outliers):
Biome 1.9.4: Checked 4511 files in 729ms.
next: Checked 4511 files in 744ms.
This PR: Checked 4511 files in 540ms.

Looks like this creates a nice boost :)
Note that both next and this PR now do the scanning upfront, but that doesn't yet result in any speedup of the analysis step of course...

Disclaimer: I don't really know whether the speedup is the result of dropping slotmap specifically, or because I dropped the "caching" of language settings, or maybe the primary speedup is less locking inside Matcher...

@github-actions github-actions bot added A-Project Area: project A-Tooling Area: internal tools labels Dec 5, 2024
@arendjr arendjr requested a review from a team December 5, 2024 11:34
Copy link

codspeed-hq bot commented Dec 5, 2024

CodSpeed Performance Report

Merging #4696 will not alter performance

Comparing arendjr:drop-slotmap (8b6eaf2) with next (f5ba162)

Summary

✅ 97 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The changes seem to be straight forward! And it seems we got a small boost too, at least we save something before slowing things done with the multi analysis :D

I just left a question

crates/biome_service/src/settings.rs Show resolved Hide resolved
@arendjr arendjr merged commit 3914be8 into biomejs:next Dec 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Project Area: project A-Tooling Area: internal tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants