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 parking_lot::Mutex instead of spin::Mutex #734

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Use parking_lot::Mutex instead of spin::Mutex #734

merged 1 commit into from
Jan 17, 2020

Conversation

ijl
Copy link
Contributor

@ijl ijl commented Jan 16, 2020

spin is no longer maintained.

Fixes #718.

spin is no longer maintained.

Fixes #718.
@ijl ijl requested a review from kngwyu January 16, 2020 13:54
@birkenfeld
Copy link
Member

So the obvious question: why didn't the original use the std Mutex -- and is parking_lot the wrong choice for the same reason?

@ijl
Copy link
Contributor Author

ijl commented Jan 16, 2020

spin and parking_lot are both faster than std. This change is least likely to cause a difference for dependent projects. For anything more, putting energy into #679 would make these locks unnecessary.

@kngwyu
Copy link
Member

kngwyu commented Jan 16, 2020

@birkenfeld
I'm not the original author of this part and don't know the intention.
Both spin and parking_lot's documents say

No poisoning, the lock is released normally on panic.

So the difference from std is they return Error when panicking or not.
And... I honestly am not sure this feature matters, since I think most of the users don't write code where multiple threads try to get GIL 🤔

@kngwyu
Copy link
Member

kngwyu commented Jan 16, 2020

@ijl
Oh, I wrote the above comment before reading your comment.

spin and parking_lot are both faster than std.

I understand..., thanks.

@kngwyu
Copy link
Member

kngwyu commented Jan 16, 2020

Sorry, I want to think about std vs parking_lot problem more.
Please wait for a while.

@kngwyu
Copy link
Member

kngwyu commented Jan 17, 2020

How we should handle LockResult is not obvious, so I decided to use parking_lot for now.
However, maybe we should remove it in the near future.

@kngwyu kngwyu merged commit db6c822 into PyO3:master Jan 17, 2020
@ijl ijl deleted the rm-spin branch January 17, 2020 11:03
@m-ou-se m-ou-se mentioned this pull request May 9, 2020
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.

spin is no longer actively maintained
3 participants