Implement rustc_datastructures::RwLock as union over RefCell and parking_lot::RwLock.#159452
Implement rustc_datastructures::RwLock as union over RefCell and parking_lot::RwLock.#159452LorrensP-2158466 wants to merge 2 commits into
rustc_datastructures::RwLock as union over RefCell and parking_lot::RwLock.#159452Conversation
…ot::RwLock`. Implementation uses the same technique as with `Lock` by using a C enum together with a union around the locking primitives.
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
#158845 replaces more cells with rwlocks, so the results for this PR should look more contrast if we benchmark it after landing that PR. |
|
Can't look at the diff rn, need to go, but @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Implement `rustc_datastructures::RwLock` as union over `RefCell` and `parking_lot::RwLock`.
Understood, but you linked the tracking issue. Do you happen to have the link to the pr in question? |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (57bfc2b): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.0%, secondary 9.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.2%, secondary -3.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.028s -> 491.544s (0.11%) |
|
Definitely better results than #136772 but not a lot of significant improvements. Though the ~10% RSS increase for I did notice that I have no And a lot less |
| unsafe fn drop_write_lock(mode: Mode, mode_union: &ModeUnion) { | ||
| // SAFETY: Caller guarantees union_access. | ||
| match mode { | ||
| Mode::NoSync => { | ||
| let borrow = unsafe { &mode_union.no_sync }; | ||
| debug_assert!(is_writing(borrow.get())); | ||
| borrow.update(|b| b + 1); |
There was a problem hiding this comment.
we do not have any Clone impls as they are not needed atm. We could in theory use borrow.replace(UNUSED) here. A very micro optimization.
| pub struct MappedReadGuard<'a, T> { | ||
| mode_union: &'a ModeUnion, | ||
| data: *const T, | ||
| marker: PhantomData<&'a T>, | ||
|
|
||
| /// The synchronization mode of the lock. This is explicitly passed to let LLVM relate it | ||
| /// to the original lock operation. | ||
| mode: Mode, | ||
| } |
There was a problem hiding this comment.
Followed the parking_lot::MappedRwLockReadGuard design:
pub struct MappedRwLockReadGuard<'a, R: RawRwLock, T: ?Sized> {
raw: &'a R, // the mode union in our case
data: *const T,
marker: PhantomData<&'a T>,
}
Revive of #136772.
cc "Parallel Rustc Front-end" #113349
Instead of explicit enums I decided to see what would happen if we would use the
ModeUnionwithMode. Nothing changes API wise, but perf may or may not improve.Second commit shows what actually changes, first commit just moves
RwLockto a separate module.