-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Optimize FromIterator for boxed slice with TrustedLen iterators #99376
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon. Please see the contribution instructions for more information. |
60f919a
to
97cee6e
Compare
I saw it generated this assembly which it wasn't there previously. Is it expected to have this new error path? Same old code but with latest stable compiler https://godbolt.org/z/Ed8fzrKcT call qword, ptr, [rip, +, _ZN5alloc7raw_vec17capacity_overflow17h752bfcb61e0e0e00E@GOTPCREL] Not quite sure if it is due to the len capacity debug assert. |
@pickfire It’s not caused by this PR: |
Wait a sec. I wonder how it passed tests on my side. Oh, just missing a couple |
This comment has been minimized.
This comment has been minimized.
Is that a regression because both beta and stable (1.62.0) is still fine? |
@pickfire This changed from beta to nightly, but I don’t think it’s a significant regression: the difference is like 10 instructions with 1 branch. |
I think ad3a791 introduced this branch and it seems deliberate. |
@the8472 Are your concerns lifted? |
Yes, #99790 should solve it. |
@the8472 Is it necessary to have the extra capacity overflow panic check? Can we somehow remove it? |
triage:
@GoldsteinE - can you answer this? |
I’m not sure. I’d rather refrain from reverting someone else’s changes without fully understanding why are they made. Anyway, the branch to the panic is needed somewhere, so I don’t think it should have a noticeable performance impact. |
You mean because the FromIterator specialization calls Extend which panics again? Yes, that code could be extracted into an unsafe function that requires the promise that sufficient capacity is available and then does the iteration without further length checks and then it could be called directly by the FromIterator and Extend impls so that FromIterator doesn't have to go through Extend anymore. I think we had that at some point, maybe it was removed due to different performance issues? Or maybe that was in a PR that didn't get merged. |
I think it should be fine, the question I asked regarding the panic wasn't introduced in this PR, was introduced in another PR. |
@m-ou-se Sorry to ping you, but what’s the state of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a correctness perspective this should be fine once the with_capacity
guarantees have been updated. But it relies a lot on the high-level API (FromIterator) doing exactly the right thing. I don't see us changing this anytime soon but I would feel better if we had an lower-level building block we could call instead which has exactly the right properties.
The Extend version of the TrustedLen specialization has already been extracted into a separate method:
rust/library/alloc/src/vec/mod.rs
Lines 2864 to 2867 in dd01a16
// specific extend for `TrustedLen` iterators, called both by the specializations | |
// and internal places where resolving specialization makes compilation slower | |
#[cfg(not(no_global_oom_handling))] | |
fn extend_trusted(&mut self, iterator: impl iter::TrustedLen<Item = T>) { |
The same could be done with the FromIterator specialization:
rust/library/alloc/src/vec/spec_from_iter_nested.rs
Lines 48 to 52 in dd01a16
impl<T, I> SpecFromIterNested<T, I> for Vec<T> | |
where | |
I: TrustedLen<Item = T>, | |
{ | |
fn from_iter(iterator: I) -> Self { |
Then we would have a method that the Box specialization can call directly and put the guarantees on its documentation.
Thanks for the review, I’ll look into possibility of using different APIs soon. |
@rustbot author |
|
On the other hand, I could just make |
b78da97
to
f0edc2d
Compare
I realized that just not using Vec makes implementation a lot simpler. @rustbot ready |
Oh well, ok, no it’s not, we lose another optimization that way. @rustbot author |
The job Click to see the possible cause of the failure (guessed by this bot)
|
@GoldsteinE any updates on this? |
@Dylan-DPC I'm kinda stuck. I don't see a way to do it without losing any optimizations or relying on behavior of Vec's FromIterator. |
I do not understand the issue from your previous comments. My last suggested action was to extract the TrustedLen specialization for FromIterator into a method and then have the vec and box specializations call that method. |
@the8472 The problem is that Vec’s
let mut vec = Vec::with_capacity(1000);
vec.push(1);
let x: Box<[_]> = vec.into_iter().collect(); which specialization should be used? We currently have test that we use the first one if possible, but it’s not exact size, so we’ll need to shrink it, potentially reallocating. |
Well, we can have multiple specializations for box, just as we do for Vec. Having a branch that may or may not reallocate on the vec -> box path seems fine. |
☔ The latest upstream changes (presumably #110331) made this pull request unmergeable. Please resolve the merge conflicts. |
@GoldsteinE @rustbot label: +S-inactive |
(Probably) closes #75636
Assembly comparison after this PR (collected with
cargo asm
):Vec<T>
This code:
Generates this assembly:
Box<[T]>
This code:
Generates this assembly: