-
Notifications
You must be signed in to change notification settings - Fork 13.3k
expose expect intrinsics + mark push slow path #17317
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
Conversation
This seems odd to only expose these macros as part of the libcore prelude, as there's basically no other core functionality that's present in the libcore prelude and not in the std prelude. Note, however, std prelude changes require an RFC. |
@alexcrichton: It's a performance optimization. These are intended to be marked as |
Oh dear, sorry for being a little slow to get back to you on this, it seems to have fallen through the cracks in my inbox! Our current policy on modifying the prelude is to require an RFC. The threshold for something going into the prelude is that it needs absolutely overwhelming evidence that it should be in the prelude. It's an unfortunate side effect that currently all macros are "in the prelude" by virtue of being a macro. (would it be possible for these to be a function?). Some examples in the past where we've required an RFC are: |
This marks the slow path in the `Vec<T>` `push` function, improviding code generation by moving it out of fast paths. The `push` function is inlined in countless locations and not all of them order these blocks appropriately without a hint. This improves the performance of the microbenchmark in #15177 by 15% when dirty page purging is disabled. Exposing the i1 version of the intrinsic for `bool` would be useful, but the quirks of Rust's code generation prevent it from becoming branch metadata so it has been left out for now. A more sophisticated approach could be taken in the future (#11092) but this provides a useful optimization feature today.
This just seems like a limitation in stability markers. It doesn't seem like @thestinger is trying to create a new public API, but the current implementation doesn't offer a way to mark the new macro as unstable or internal. It doesn't seem like that should block this performance improvement. |
It can't be a function because |
@wycats yes, it's not supposed to be a public API, but that's the way macros work right now. As-is we require an RFC to modify the prelude (which includes macros right now), and we have precedent for holding off small performance improvements such as this because of macros (see #17857 and #16204). Yes, this is unfortunate, and it's not the state of affairs we'd like to have, but it's an unavoidable reality with today's macro system. |
I'll put these in an |
This marks the slow path in the
Vec<T>
push
function, improvidingcode generation by moving it out of fast paths. The
push
function isinlined in countless locations and not all of them order these blocks
appropriately without a hint. This improves the performance of the
microbenchmark in #15177 by 15% when dirty page purging is disabled.
Exposing the i1 version of the intrinsic for
bool
would be useful, butthe quirks of Rust's code generation prevent it from becoming branch
metadata so it has been left out for now.
A more sophisticated approach could be taken in the future (#11092) but
this provides a useful optimization feature today.