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

use Mutex instead of RwLock for securing the inner data #67

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

hozan23
Copy link
Contributor

@hozan23 hozan23 commented Jun 27, 2024

This addresses the issue #42

The use of RwLock is typically used when there is a need for multiple readers and infrequent writers, as RwLock allows multiple concurrent readers or a single writer. However, most functions were using the write() method, while the read() method was rarely used.

Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM but could you please add a brief rationale in the PR and commit description (git commit --amend && git push -f YOUR_REPO ues_mutex_instead_of_rwlock in case you didn't know how to edit commits) ? While you do that, please also append Fixes #42. to the commit message too?

The use of `RwLock` is typically used when there is a need for
multiple readers and infrequent writers, as `RwLock` allows multiple
concurrent readers or a single writer. However, most functions were
using the `write()` method, while the `read()` method was rarely used.

Fixes smol-rs#42
@hozan23 hozan23 force-pushed the ues_mutex_instead_of_rwlock branch from 37fac1e to 6b8a513 Compare June 27, 2024 10:59
@hozan23 hozan23 requested a review from zeenix June 27, 2024 11:01
@hozan23
Copy link
Contributor Author

hozan23 commented Jun 27, 2024

@zeenix thanks, I updated the commit message.

Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Awesome. I'm merge now but please don't forget to also update the PR description.

@zeenix zeenix merged commit ef37a05 into smol-rs:master Jun 27, 2024
5 checks passed
@fogti
Copy link
Member

fogti commented Jun 27, 2024

do we have some benchmarks to check that this doesn't cause sigificant performance regressions?

@zeenix
Copy link
Member

zeenix commented Jun 27, 2024

do we have some benchmarks to check that this doesn't cause sigificant performance regressions?

No but is there a reason we'd suspect so? 🤔

@hozan23
Copy link
Contributor Author

hozan23 commented Jun 27, 2024

@fogti In theory, it should not cause any performance regressions. #42 didn't mention any performance issues with using rwlock, but as far as I know, the only issue with rwlock is writer starvation, whereas a mutex cannot have this issue. However, I agree that we do need some benchmarks to check the performance. I will do it when I have time.

@fogti
Copy link
Member

fogti commented Jun 27, 2024

@hozan23 tbh, the link to the introduction of rwlock suffices for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants