Skip to content

Unified lock types for rustpython_common #2239

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

Merged
merged 3 commits into from
Oct 8, 2020

Conversation

coolreader18
Copy link
Member

@coolreader18 coolreader18 commented Sep 27, 2020

For #2226 and specifically in order to implement BorrowedValue::map(); I realized that we can just make our own RawMutex/RawRwLock and use it to define the Mutex types. These should be faster, too, since RefCell has more overhead (immutable accessor count mainly, Cell<isize> vs Cell<bool>) since they're more direct with the use case -- e.g. just one writer vs RefCell supports multiple with RefMut::map_split. Turns out we can't actually make it a bool, since we subtly depend on PyRwLock having a recursive locking behavior, even though that's not guaranteed by parking_lot -- "Note that attempts to recursively acquire a read lock on a RwLock when the current thread already holds one may result in a deadlock."

@coolreader18 coolreader18 force-pushed the coolreader18/unified-common-locks branch from 9a7e027 to 7a39500 Compare September 27, 2020 04:40
mod cell_lock;
pub use cell_lock::{RawCellMutex as RawMutex, RawCellRwLock as RawRwLock};

pub use once_cell::unsync::{Lazy, OnceCell};
Copy link
Member

Choose a reason for hiding this comment

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

is this also belong to lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense there; sync::OnceCell at least has to lock when it's initializing

Copy link
Member Author

Choose a reason for hiding this comment

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

And it was there when this was cell.rs as well

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

I am not understanding this very well. Any other reviewer for this?

If possible, merging #2226 first maybe helpful. @qingshi163 experienced a lot of rebasing for that already

@coolreader18
Copy link
Member Author

I suppose so, I'd just like to have them merged close together so that we don't run into issues with the soundness hole.

@coolreader18
Copy link
Member Author

I'm gonna merge this; I feel like cell_lock is fairly easy to maintain/understand if you just look at it in isolation.

@coolreader18 coolreader18 merged commit e2bc53f into master Oct 8, 2020
@coolreader18 coolreader18 deleted the coolreader18/unified-common-locks branch October 8, 2020 00:36
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