Skip to content

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.

3 participants