Turn ZeroVec into a struct #2599
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Manishearth
requested review from
sffc,
robertbastian,
zbraniecki,
gregtatum,
nordzilla and
a team
as code owners
|
Running norm benches (with non icu4x benches commented out) gives me: after this PRbefore this PRI did run the benches post-PR first, so the negative deltas are good. |
robertbastian
requested changes
Sep 21, 2022
| /// zerovec.with_mut(|v| v.push(12_u16.to_unaligned())); | ||
| /// assert!(zerovec.is_owned()); | ||
| /// ``` | ||
| pub fn with_mut<R>(&mut self, f: impl FnOnce(&mut Vec<T::ULE>) -> R) -> R { |
There was a problem hiding this comment.
I'm not a fan of the continuation-passing style. Many uses of this actually only need a mutable slice, which is something we can return. Other places, like in datagen, do things like pushing by repeatedly calling with_mut which I think is not very readable. They should assemble a Vec and then convert it into a ZeroVec at the end.
There was a problem hiding this comment.
Good point, added. I didn't move people off of with_mut, this might be a change we can do later. Ultimately I think such an API is still valuable to have.
robertbastian
previously approved these changes
Sep 21, 2022
robertbastian
previously approved these changes
Sep 21, 2022
robertbastian
approved these changes
Sep 21, 2022
|
wasm build failed due to flakiness |
Manishearth
removed request for
a team,
zbraniecki,
gregtatum and
nordzilla
sffc
reviewed
Sep 23, 2022
| where | ||
| T: AsULE + ?Sized, | ||
| T: AsULE, |
There was a problem hiding this comment.
Question: Why did you drop the ?Sized ?
There was a problem hiding this comment.
It's completely useless, this is for Copy stack types, there's no way to have a ?Sized type in ULE and have it work. The ULE methods won't be callable for such types.
Most of the ZeroVec methods also can't have ?Sized bounds
There was a problem hiding this comment.
But this is the AsULE. I can't think of a specific use case, but it doesn't seem that T: Sized needs to be a requirement
There was a problem hiding this comment.
AsULE returns owned values, so to do anything useful it has to be Sized
In general the idea is to be as generic as possible in trait bounds where more specific bounds are unnecessary, but I don't think that advice applies for opt out bounds like ?Sized, "be as generic as possible" is shorthand for "don't use unnecessary bounds"
| ZeroVec::Owned(vec) => &**vec, | ||
| ZeroVec::Borrowed(slice) => *slice, | ||
| }; | ||
| let slice = unsafe { &*self.buf }; |
There was a problem hiding this comment.
Nit: Safety comment. It's not super clear what is being cast from and to.
Comment on lines
+100
to
+101
| unsafe impl<'a, T: AsULE> Send for ZeroVec<'a, T> where T::ULE: Send + Sync {} | ||
| unsafe impl<'a, T: AsULE> Sync for ZeroVec<'a, T> where T::ULE: Sync {} |
There was a problem hiding this comment.
Nit: Safety docs. Why does T::ULE: Sync imply ZeroVec<'a, T>: Sync?
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust,ignore |
There was a problem hiding this comment.
Nit: Don't ignore this example. Make it so that it runs and passes.
There was a problem hiding this comment.
huh, i wodner why this was ignore initially
| /// ``` | ||
| Borrowed(&'a [T::ULE]), | ||
| /// Pointer to data | ||
| buf: *mut [T::ULE], |
There was a problem hiding this comment.
Issue: Don't you need a manual Drop impl to clean this up? (I think it can just call self.into_cow())
sffc
reviewed
Sep 23, 2022
| @@ -433,10 +454,28 @@ where | |||
| let ptr = ptr as *mut P::ULE; | |||
| Vec::from_raw_parts(ptr, len, cap) | |||
There was a problem hiding this comment.
Suggestion: There are now multiple places where we convert back and forth between a vec and its raw parts (here, into_cow, and new_owned). Consider making them share code or at least use the same style with similar safety comments.
There was a problem hiding this comment.
They already share code as much as possible; we deconstruct the vec in one place (new_owned) and construct it once (into_cow). There's also try_into_converted but that's about constructing a vec of a different type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This avoids branches in most cases by avoiding the enum discriminant check, instead stuffing the discriminant into the zero-ness of the vec.
Supersedes #2554
Seems to give a ~10% improvement, but some of our benches are flaky.