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

Replace std::sync locks with parking_lot locks #9731

Merged
merged 2 commits into from
May 23, 2024

Conversation

marcins
Copy link
Contributor

@marcins marcins commented May 22, 2024

↪️ Pull Request

Internally we see crashes related to locks, not coming from Rust, but from the system level pthread calls. This seems to be mostly related to the resolver, and exhibits itself as a system error 22 - which means a lock operation was called with an "invalid argument".

In Rust 1.78 the RwLock implementation, where we saw these crashes occurring, was replaced with a native rust implementation. We've upgraded to a build built with Rust 1.78, and now see crashes coming from the Mutex in https://github.com/parcel-bundler/parcel/blob/v2/packages/utils/node-resolver-rs/src/cache.rs#L196. These are of the form:
failed to lock mutex: Invalid argument (os error 22). This error is not readily reproducible for debugging, and doesn't seem to be reproducible under a stress test of the resolver either.

This change replaces the std::sync usage of locks with the parking_lot implementation. The hope is that this makes the errors go away, or manifest themselves in a form that provides better insight into why they are occurring. parking_lot is also advertised as having better perf, so it's worth trying to see if we see any improvement there too as a side beenfit.

🚨 Test instructions

We've tested against our very large application internally. Haven't really seen a performance boost, but no performance regression either.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@marcins marcins marked this pull request as ready for review May 23, 2024 03:40
@marcins marcins requested a review from a team May 23, 2024 03:40
@marcins marcins enabled auto-merge May 23, 2024 06:37
@marcins marcins added this pull request to the merge queue May 23, 2024
Merged via the queue into v2 with commit e1d3cbe May 23, 2024
15 of 17 checks passed
@mischnic mischnic deleted the mszczepanski/use-parking_lot-locks branch May 28, 2024 11:37
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