-
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
attempt to optimise vectored write #98324
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
(rust-highfive has picked a reviewer for you, use r? to override) |
vec.spare_capacity_mut().get_unchecked_mut(i).write(0); | ||
} | ||
vec.set_len(pos); | ||
} | ||
} |
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.
Hm, it seems to me that I'd expect the sequence here (reserve + memset) to be pretty much exactly what resize(pos, 0)
compiles to. Is that not the case today? Should we be fixing this function, and not whatever makes resize optimize poorly?
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.
Certainly a good idea. The issue with resize is that it directly calls extend_with_slice, which always calls reserve and then does this exact assign loop.
The optimiser is very bad at optimising out allocations. But maybe there is a way here to make it slightly more effective.
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.
I've put together this incredibly simple example: https://godbolt.org/z/bobosGa9r to demonstrate that it always double-allocs. I'm not too adept to how the lowering works, I guess somewhere it inlines late and doesn't have the correct information still lying around to determine that one of the branches is redundant
This comment has been minimized.
This comment has been minimized.
Can you squash the commits into one? I think with that this seems OK to merge. |
@rustbot author |
d055069
to
803083a
Compare
@rustbot ready |
@bors r+ rollup=never |
📌 Commit 803083a has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (64eb9ab): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
benchmarked:
old:
new:
More unsafe than I wanted (and less gains) in the end, but it still does the job