Revamp CachePadded #1
Conversation
ghost
requested a review
from 
ghost
mentioned this pull request
|
I'm not 100% up to date on the union story, but could we make the nightly version use |
|
Thanks for taking a look! Yeah, that's a good idea. I've pushed a commit with |
|
Nice! I think this is the best way forward 👍 |
src/cache_padded.rs
Outdated
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "CachePadded {{ ... }}") | ||
| } | ||
| /// `[T; 0]` ensures correct alignment. |
There was a problem hiding this comment.
Is it a documented behavior?
There was a problem hiding this comment.
I didn't find anything explicit on this issue, but FWIW, there's this: link
src/cache_padded.rs
Outdated
| #[repr(align(64))] | ||
| #[allow(unions_with_drop_fields)] | ||
| union Inner<T> { | ||
| bytes: [u8; CACHE_LINE_BYTES], |
There was a problem hiding this comment.
May I ask why do we need it here?
There was a problem hiding this comment.
Good point, looks like align is enough in nightly (https://play.rust-lang.org/?gist=4c3841431f1d27ff7997eb4f2c2a2471&version=nightly) so I think we can get away without the union at all.
src/cache_padded.rs
Outdated
| _marker: ([], PhantomData), | ||
| }; | ||
| let p: *mut T = &mut *padded; | ||
| ptr::write(p, t); |
There was a problem hiding this comment.
I want to make sure that this write conforms to C's union semantics. (I'm assuming Rust's union is the same with C's union.) Unfortunately I don't know the exact definition in the standard C: I just know that there is a very complex rule that determines what is safe and what is unsafe use of unions. A good reference would be Krebber's thesis.
Even if it turned out to be inconformant to C, it's probably okay, because for now we only need to think of LLVM semantics. We don't need to think of the rules on unions when we turn off the -tbaa option (type-based alias analysis).
|
Changes:
|
|
@jeehoonkang Can we merge? |
| data: UnsafeCell<[usize; CACHE_LINE]>, | ||
| _marker: ([Padding; 0], marker::PhantomData<T>), | ||
| } | ||
| cfg_if! { |
| cfg_if! { | ||
| if #[cfg(feature = "nightly")] { | ||
| #[derive(Clone)] | ||
| #[repr(align(64))] |
There was a problem hiding this comment.
We use a magic number here because we cannot use a constant, right?
Changes:
ZerosValid.repr(align(64))with thenightlyfeature in order to align to cache line length.DropforCachePadded<T>.CloneforCachePadded<T>.DebugforCachePadded<T>.From<T>forCachePadded<T>.