Remove `AccumulateVec` and its uses. by nnethercote · Pull Request #53659 · rust-lang/rust

nnethercote · 2018-08-24T06:33:39Z

It's basically just a less capable version of SmallVec.

FWIW, the only use of ArrayVec is now within HybridIdxSet.

r? @Mark-Simulacrum

Mark-Simulacrum · 2018-08-24T12:09:50Z

Did this forget the removal of the accumulate_vec file itself?

nnethercote · 2018-08-24T23:18:47Z

Did this forget the removal of the accumulate_vec file itself?

Yes! I will fix that on Monday.

bors · 2018-08-25T00:58:21Z

☔ The latest upstream changes (presumably #53225) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2018-08-26T05:25:18Z

IIRC, SmallVec had a harder time optimizing, because AccumulateVec never switches from "inline" to "heap" as it grows, the decision is fixed when starting to fill it from an iterator, hence "accumulate".

Please do not merge such PRs without running perf first. I'd be kind of shocked to see that this PR doesn't affect anything, because IIRC we tried a few things and this was the only good one.

eddyb · 2018-08-26T05:26:38Z

Also, could it be possible to be cc'd on PRs that involve performance-related code? I only saw this PR randomly. Maybe the whole of @rust-lang/wg-compiler-performance? (but others might not like it)

Mark-Simulacrum · 2018-08-26T13:45:34Z

@bors try

bors · 2018-08-26T13:45:39Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout rm-AccumulateVec (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self rm-AccumulateVec --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/librustc_typeck/lib.rs
Auto-merging src/librustc_typeck/astconv.rs
Auto-merging src/librustc/ty/subst.rs
CONFLICT (content): Merge conflict in src/librustc/ty/subst.rs
Auto-merging src/librustc/ty/context.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

nnethercote · 2018-08-27T02:59:15Z

@bors try

bors · 2018-08-27T02:59:20Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout rm-AccumulateVec (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self rm-AccumulateVec --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/librustc_typeck/lib.rs
Auto-merging src/librustc_typeck/astconv.rs
Removing src/librustc_data_structures/accumulate_vec.rs
Auto-merging src/librustc/ty/subst.rs
CONFLICT (content): Merge conflict in src/librustc/ty/subst.rs
Auto-merging src/librustc/ty/context.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

nnethercote · 2018-08-27T04:17:41Z

@bors try

bors · 2018-08-27T04:17:52Z

⌛ Trying commit 1b238a1dabf00e11fb74ed5547d724163b0e1cc9 with merge 44c5832a6fd7376b9c576e00084ae482906b4bb1...

rust-highfive · 2018-08-27T04:22:13Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

#######################################################################   99.7%
######################################################################## 100.0%
[00:01:58] extracting /checkout/obj/build/cache/2018-08-01/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:00]     Updating registry `https://github.com/rust-lang/crates.io-index`
[00:02:24] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:24] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:24] Makefile:81: recipe for target 'prepare' failed
[00:02:24] make: *** [prepare] Error 1
[00:02:25] Command failed. Attempt 2/5:
[00:02:25] Command failed. Attempt 2/5:
[00:02:27] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:27] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:27] Makefile:81: recipe for target 'prepare' failed
[00:02:27] make: *** [prepare] Error 1
[00:02:29] Command failed. Attempt 3/5:
[00:02:29] Command failed. Attempt 3/5:
[00:02:29] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:29] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:29] Makefile:81: recipe for target 'prepare' failed
[00:02:29] make: *** [prepare] Error 1
[00:02:32] Command failed. Attempt 4/5:
[00:02:32] Command failed. Attempt 4/5:
[00:02:32] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:32] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:32] Makefile:81: recipe for target 'prepare' failed
[00:02:32] make: *** [prepare] Error 1
[00:02:36] Command failed. Attempt 5/5:
[00:02:36] Command failed. Attempt 5/5:
[00:02:38] error: the lock file needs to be updated but --locked was passed to prevent this
[00:02:38] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:38] Makefile:81: recipe for target 'prepare' failed
[00:02:38] make: *** [prepare] Error 1
[00:02:38] The command has failed after 5 attempts.
travis_time:end:076feb4c:start=1535343529626126104,finish=1535343691838727626,duration=162212601522
---
travis_time:end:21582c00:start=1535343692350487547,finish=1535343692362633227,duration=12145680
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:008ffebc
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:08e3b4ab
travis_time:start:08e3b4ab
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1105f73f
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

nnethercote · 2018-08-27T04:22:50Z

@bors try

bors · 2018-08-27T04:23:00Z

⌛ Trying commit 9c35961 with merge b076004...

@Mark-Simulacrum

Remove `AccumulateVec` and its uses. It's basically just a less capable version of `SmallVec`. FWIW, the only use of `ArrayVec` is now within `HybridIdxSet`. r? @Mark-Simulacrum

rust-highfive · 2018-08-27T04:25:45Z

The job dist-x86_64-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_fold:end:services

travis_fold:start:git.checkout
travis_time:start:108b9853
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
############                                                              17.7%
######################################################################## 100.0%
[00:04:00] extracting /checkout/obj/build/cache/2018-08-01/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:04:02]     Updating registry `https://github.com/rust-lang/crates.io-index`
[00:04:27] error: the lock file needs to be updated but --locked was passed to prevent this
[00:04:27] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:04:27] make: *** [prepare] Error 1
[00:04:28] Command failed. Attempt 2/5:
[00:04:28] Command failed. Attempt 2/5:
[00:04:30] error: the lock file needs to be updated but --locked was passed to prevent this
[00:04:30] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:04:30] make: *** [prepare] Error 1
[00:04:32] Command failed. Attempt 3/5:
[00:04:32] Command failed. Attempt 3/5:
[00:04:33] error: the lock file needs to be updated but --locked was passed to prevent this
[00:04:33] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:04:33] make: *** [prepare] Error 1
[00:04:36] Command failed. Attempt 4/5:
[00:04:36] Command failed. Attempt 4/5:
[00:04:36] error: the lock file needs to be updated but --locked was passed to prevent this
[00:04:36] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:04:36] make: *** [prepare] Error 1
[00:04:40] Command failed. Attempt 5/5:
[00:04:40] Command failed. Attempt 5/5:
[00:04:40] error: the lock file needs to be updated but --locked was passed to prevent this
[00:04:40] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:04:40] make: *** [prepare] Error 1
[00:04:40] The command has failed after 5 attempts.
travis_time:end:26e94942:start=1535343640974327068,finish=1535343939862089430,duration=298887762362

---
travis_time:end:3274f410:start=1535343940307294801,finish=1535343940315297174,duration=8002373
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0236b348
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:274d92c3
travis_time:start:274d92c3
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0b5e2c99
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

Zoxc · 2018-08-27T04:51:15Z

@nnethercote Rebase and run x.py build locally to update Cargo.lock

bors · 2018-08-27T06:15:59Z

☀️ Test successful - status-travis
State: approved= try=True

nnethercote · 2018-08-27T09:06:22Z

I'm confused... I definitely already did ./x.py build locally, and the Cargo.lock file is updated appropriately within the commit, AFAICT. And the try push succeeded...

nnethercote · 2018-08-27T09:06:56Z

@rust-timer build b076004

rust-timer · 2018-08-27T09:06:57Z

Success: Queued b076004 with parent 3a2c603, comparison URL.

nnethercote · 2018-08-27T23:32:29Z

The perf results all look like noise. Unsurprising, because rustc now uses the SmallVec from crates.io, which has the same optimization when creating from an iterator that AccumulateVec has.

r? @Mark-Simulacrum

Mark-Simulacrum · 2018-08-28T00:17:37Z

Agreed that perf is probably noise. @bors r+

bors · 2018-08-28T00:17:38Z

📌 Commit 9c35961 has been approved by Mark-Simulacrum

Mark-Simulacrum · 2018-08-28T00:17:41Z

Agreed that perf is probably noise. @bors r+

Mark-Simulacrum · 2018-08-28T00:17:48Z

Agreed that perf is probably noise. @bors r+

Mark-Simulacrum · 2018-08-28T01:51:18Z

Agreed that perf is probably noise. @bors r+

bors · 2018-08-28T01:51:18Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Warn on anon params in 2015 edition #53272

bors · 2018-08-28T01:51:19Z

📌 Commit 9c35961 has been approved by Mark-Simulacrum

eddyb · 2018-08-28T05:29:16Z

Unsurprising, because rustc now uses the SmallVec from crates.io, which has the same optimization when creating from an iterator that AccumulateVec has.

Oh, that makes sense. I didn't realize that happened!

bors · 2018-08-28T15:15:51Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout rm-AccumulateVec (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self rm-AccumulateVec --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/librustc_typeck/lib.rs
Auto-merging src/librustc_data_structures/lib.rs
Removing src/librustc_data_structures/accumulate_vec.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

It's basically just a less capable version of `SmallVec`.

nnethercote · 2018-08-29T01:03:49Z

@bors r=Mark-Simulacrum

bors · 2018-08-29T01:03:49Z

@nnethercote: 🔑 Insufficient privileges: Not in reviewers

Mark-Simulacrum · 2018-08-29T02:12:13Z

@bors r+ delegate+

bors · 2018-08-29T02:12:14Z

✌️ @nnethercote can now approve this pull request

bors · 2018-08-29T02:12:15Z

📌 Commit 8cecfa6 has been approved by Mark-Simulacrum

bors · 2018-08-29T04:20:08Z

⌛ Testing commit 8cecfa6 with merge 29e6aab...

@Mark-Simulacrum

Remove `AccumulateVec` and its uses. It's basically just a less capable version of `SmallVec`. FWIW, the only use of `ArrayVec` is now within `HybridIdxSet`. r? @Mark-Simulacrum

bors · 2018-08-29T06:24:24Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 29e6aab to master...

rust-highfive assigned Mark-Simulacrum Aug 24, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2018

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2018

nnethercote force-pushed the rm-AccumulateVec branch from de3f1aa to 8c10165 Compare August 27, 2018 02:58

nnethercote force-pushed the rm-AccumulateVec branch from 8c10165 to 1b238a1 Compare August 27, 2018 04:17

nnethercote force-pushed the rm-AccumulateVec branch from 1b238a1 to 9c35961 Compare August 27, 2018 04:22

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 28, 2018

Remove AccumulateVec and its uses.

8cecfa6

It's basically just a less capable version of `SmallVec`.

nnethercote force-pushed the rm-AccumulateVec branch from 9c35961 to 8cecfa6 Compare August 28, 2018 22:51

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2018

bors merged commit 8cecfa6 into rust-lang:master Aug 29, 2018

nnethercote deleted the rm-AccumulateVec branch August 29, 2018 06:49

Uh oh!

Conversation

nnethercote commented Aug 24, 2018

Uh oh!

Mark-Simulacrum commented Aug 24, 2018

Uh oh!

nnethercote commented Aug 24, 2018

Uh oh!

bors commented Aug 25, 2018

Uh oh!

eddyb commented Aug 26, 2018

Uh oh!

eddyb commented Aug 26, 2018

Uh oh!

Mark-Simulacrum commented Aug 26, 2018

Uh oh!

bors commented Aug 26, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

bors commented Aug 27, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

bors commented Aug 27, 2018

Uh oh!

rust-highfive commented Aug 27, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

bors commented Aug 27, 2018

Uh oh!

rust-highfive commented Aug 27, 2018

Uh oh!

Zoxc commented Aug 27, 2018

Uh oh!

bors commented Aug 27, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

rust-timer commented Aug 27, 2018

Uh oh!

nnethercote commented Aug 27, 2018

Uh oh!

Mark-Simulacrum commented Aug 28, 2018

Uh oh!

bors commented Aug 28, 2018

Uh oh!

Mark-Simulacrum commented Aug 28, 2018

Uh oh!

Mark-Simulacrum commented Aug 28, 2018

Uh oh!

Mark-Simulacrum commented Aug 28, 2018

Uh oh!

bors commented Aug 28, 2018

Uh oh!

bors commented Aug 28, 2018

Uh oh!

eddyb commented Aug 28, 2018

Uh oh!

bors commented Aug 28, 2018

Uh oh!

nnethercote commented Aug 29, 2018

Uh oh!

bors commented Aug 29, 2018

Uh oh!

Mark-Simulacrum commented Aug 29, 2018

Uh oh!

bors commented Aug 29, 2018

Uh oh!

bors commented Aug 29, 2018

Uh oh!

bors commented Aug 29, 2018

Uh oh!

bors commented Aug 29, 2018

Uh oh!

Reviewers

Assignees