Reoptimize VecDeque::append #53564
Reoptimize VecDeque::append #53564
Conversation
rust-highfive
added
the
S-waiting-on-review
Oh wow, that's really bad! FYI: |
|
In fact, after rereading the Rustonomicon, I found that creating slices of unitialized values if perfectly fine, it's just forbidden to read or write to it without using |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@MaloJaffre Interesting, that appears to be true. I thought that references and slices being marked |
|
/cc @RalfJung |
|
Is the simplest thing to just revert the PR which introduced this until such time as it can be fixed? |
|
@shepmaster Considering that nightly is pretty widely used, that might not be a bad idea. I made some more attempts at trying to reproduce the original problem in a standalone manner, but so far I have not managed to succeed in doing this. |
|
That PR should be reverted as soon as we can; if someone can prepare a PR that would be great. |
|
miri is still WIP so if you'd prefer I can double-check to see if this looks like a false positive. But the fact that real crashes occur means, I guess, that it might be legit... |
|
Ah, I think what is going on is that the Concretely, it first turns the entire buffer into a slice ( |
|
I will open a reverting PR soon, along with a fix for #53566. |
|
I’m sorry, I feel I won’t be able to complete this review in reasonable time frame. Could someone else (perhaps from @rust-lang/libs) take over? |
|
If #52553 made things worse perhaps we could revert it in the meantime, even if that’s not a complete fix. |
|
@SimonSapin see #53571 |
|
To prevent this falling through the cracks, randomly reassigning to... |
|
☔ The latest upstream changes (presumably #53530) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I think reverting my PR for now is a good idea but I'm not sure that this PR is needed to fix the segfault. See my comment on the issue for what I think is the reason for the segfault and what would be a much simpler fix. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
…onSapin" This partially reverts commit d5b6b95, reversing changes made to 6b1ff19. Fixes rust-lang#53529. Cc: rust-lang#53564.
|
@bors: r=gnzlbg |
|
📌 Commit 21d2a6c has been approved by |
bors
added
S-waiting-on-bors
Reoptimize VecDeque::append ~Unfortunately, I don't know if these changes fix the unsoundness mentioned in #53529, so it is stil a WIP. This is also completely untested. The VecDeque code contains other unsound code: one example : [reading unitialized memory](https://play.rust-lang.org/?gist=6ff47551769af61fd8adc45c44010887&version=nightly&mode=release&edition=2015) (detected by MIRI), so I think this code will need a bigger refactor to make it clearer and safer.~ Note: this is based on #53571. r? @SimonSapin Cc: #53529 #52553 @yorickpeterse @jonas-schievink @Pazzaz @shepmaster.
|
☀️ Test successful - status-appveyor, status-travis |
|
The good news is that miri has nothing to complain about when running the same test case with the new |
|
What is the impact of this change? |
|
@jens1o it is a fix to an earlier attempt to speed up |
|
@pnkfelix has done some awesome bug hunting to pinpoint a 1.30.0 regression to this PR. I've posted a revert while it's investigated, but we can of course avoid reverting if the bug is found sooner! |
…ckler Fix a regression in 1.30 by reverting rust-lang#53564 Investigation on rust-lang#54477 revealed rust-lang#53564 as the culprit in the regression for that crate. I've reproduced the regression with the [detailed test cases provided](rust-lang#54477 (comment)). While we figure out how to fix the regression this commit reverts the current culprit.
Rollup of 11 pull requests Successful merges: - #54078 (Expand the documentation for the `std::sync` module) - #54717 (Cleanup rustc/ty part 1) - #54781 (Add examples to `TyKind::FnDef` and `TyKind::FnPtr` docs) - #54787 (Only warn about unused `mut` in user-written code) - #54804 (add suggestion for inverted function parameters) - #54812 (Regression test for #32382.) - #54833 (make `Parser::parse_foreign_item()` return a foreign item or error) - #54834 (rustdoc: overflow:auto doesn't work nicely on small screens) - #54838 (Fix typo in src/libsyntax/parse/parser.rs) - #54851 (Fix a regression in 1.30 by reverting #53564) - #54853 (Remove unneccessary error from test, revealing NLL error.) Failed merges: r? @ghost
|
|
||
| // This is only safe because copy_slice never panics when capacity is sufficient. | ||
| self.copy_slice(src_low); | ||
| self.copy_slice(src_high); |
There was a problem hiding this comment.
Are these two lines in the wrong order? src_high should be appended before src_low.
…ts, r=nikomatsakis Regression tests for issue rust-lang#54477. At some point someone may want to revisit PR rust-lang#53564 it would be really good to have regression tests for rust-lang#54477 before that happens. :)
Optimize VecDeque::append Optimize `VecDeque::append` to do unsafe copy rather than iterating through each element. On my `Intel(R) Xeon(R) CPU E5-2630 v4 @ 2.20GHz`, the benchmark shows 37% improvements: ``` Master: custom-bench vec_deque_append 583164 ns/iter custom-bench vec_deque_append 550040 ns/iter Patched: custom-bench vec_deque_append 349204 ns/iter custom-bench vec_deque_append 368164 ns/iter ``` Additional notes on the context: this is the third attempt to implement a non-trivial version of `VecDeque::append`, the last two are reverted due to unsoundness or regression, see: - rust-lang#52553, reverted in rust-lang#53571 - rust-lang#53564, reverted in rust-lang#54851 Both cases are covered by existing tests. Signed-off-by: tabokie <[email protected]>
Unfortunately, I don't know if these changes fix the unsoundness mentioned in #53529, so it is stil a WIP.This is also completely untested.
The VecDeque code contains other unsound code: one example : reading unitialized memory (detected by MIRI), so I think this code will need a bigger refactor to make it clearer and safer.
Note: this is based on #53571.
r? @SimonSapin
Cc: #53529 #52553 @yorickpeterse @jonas-schievink @Pazzaz @shepmaster.