-
Notifications
You must be signed in to change notification settings - Fork 150
Some minor grammar fixes, make squeue's push
functions consume each entry
instead of cloning it, and make push_unchecked
public
#326
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Do you have any benchmarks proving this? we need to copy the entry from application's memory (either stack or heap) to ring, which is always an unavoidable memcpy. Using move does not change this. If you want to optimize push entry, should look at this #116
Do you have any need to use it? Also, please don't put too many unrelated changes in one PR. |
I apologize for the PR overloading. Perhaps performance wasn't the right way of proposing this change. It is trivially obvious that this cloning paradigm will only ever create more instructions for the processor to execute, and will only ever incur more copying than moving the value if the compiler cannot optimize away the As for |
I'm willing to expose I don't see any real benefit in using move instead of reference, and it would break the API, so it won't be accepted any time soon. I think by make |
I propose that #[inline]
pub unsafe fn push_multiple<T, I>(&mut self, entries: T) -> Result<(), PushError>
where
I: ExactSizeIterator<Item = E>,
T: IntoIterator<Item = E, IntoIter = I>,
{
let iter = entries.into_iter();
if self.capacity() - self.len() < iter.len() {
return Err(PushError);
}
for entry in iter {
self.push_unchecked(entry);
}
Ok(())
} The only change this induces is it makes the input |
It is strange to take a reference to an entry and then immediately cloning it, it should be at the developers' discretion whether or not they would like their entry cloned. For low-latency operations this unnecessary cloning can be expensive and is completely unnessecary as far as I can tell.
I don't know why
push_unchecked
was ever private; it should again be at the developer's discretion whether or not to useunchecked
functions.I initially thought that it had something to do with keeping the integrity of the Entry intact, that dropping the entry would somehow make it unviable, but the clone is obviously immediately dropped anyway...