Use optimized SmallVec implementation #53384

gootorov · 2018-08-15T11:04:22Z

This PR replaces current SmallVec implementation with the one from the Servo project.

rust-highfive · 2018-08-15T11:04:31Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

gootorov · 2018-08-15T11:21:05Z

New smallvec is quite faster, but only when the number of elements is large, otherwise it is about the same as before. It would be interesting to see full perf comparison.

Current smallvec:

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:          35 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          40 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:          37 ns/iter (+/- 4)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          34 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:          29 ns/iter (+/- 7)
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          54 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:          50 ns/iter (+/- 8)
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          36 ns/iter (+/- 1)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          41 ns/iter (+/- 34)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:          37 ns/iter (+/- 4)

New smallvec:

test small_vec::tests::fill_small_vec_1_10_with_cap  ... bench:          33 ns/iter (+/- 6)
test small_vec::tests::fill_small_vec_1_10_wo_cap    ... bench:          30 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_1_50_with_cap  ... bench:          43 ns/iter (+/- 5)
test small_vec::tests::fill_small_vec_1_50_wo_cap    ... bench:          34 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_32_10_with_cap ... bench:          14 ns/iter (+/- 1) // Faster
test small_vec::tests::fill_small_vec_32_10_wo_cap   ... bench:           8 ns/iter (+/- 0) // Faster
test small_vec::tests::fill_small_vec_32_50_with_cap ... bench:          38 ns/iter (+/- 5) // Faster
test small_vec::tests::fill_small_vec_32_50_wo_cap   ... bench:          34 ns/iter (+/- 3) // Faster
test small_vec::tests::fill_small_vec_8_10_with_cap  ... bench:          32 ns/iter (+/- 2)
test small_vec::tests::fill_small_vec_8_10_wo_cap    ... bench:          30 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_8_50_with_cap  ... bench:          36 ns/iter (+/- 3)
test small_vec::tests::fill_small_vec_8_50_wo_cap    ... bench:          35 ns/iter (+/- 3)

Mark-Simulacrum · 2018-08-15T12:00:25Z

@bors try

bors · 2018-08-15T12:00:36Z

⌛ Trying commit 32a928feb6b6541d9198b09594c3f1e9e051d781 with merge 31e325e55811a6bde18f86114c56f04729c78575...

bors · 2018-08-15T14:04:44Z

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

gootorov · 2018-08-17T19:47:03Z

Rebased.

arthurprs · 2018-08-18T22:24:04Z

Thanks for this. I think more important than the cycles, it is 4/8 bytes smaller.

Mark-Simulacrum · 2018-08-18T22:25:38Z

@rust-timer build 31e325e55811a6bde18f86114c56f04729c78575

rust-timer · 2018-08-18T22:25:39Z

Success: Queued 31e325e55811a6bde18f86114c56f04729c78575 with parent 81cfaad, comparison URL.

bors · 2018-08-19T23:11:13Z

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

gootorov · 2018-08-20T09:06:41Z

Rebased.

Mark-Simulacrum · 2018-08-20T11:54:30Z

Perf results look fine to me, will try to get a review in sometime today or tomorrow.

llogiq · 2018-08-21T11:03:37Z

src/librustc/Cargo.toml

@@ -33,6 +33,7 @@ parking_lot = "0.5.5"
 byteorder = { version = "1.1", features = ["i128"]}
 chalk-engine = { version = "0.7.0", default-features=false }
 rustc_fs_util = { path = "../librustc_fs_util" }
+smallvec = "0.6.4"


We may want to use the union feature here. (smallvec = { version = "0.6.4", features = ["union"] }).

llogiq · 2018-08-21T11:04:49Z

src/librustc_allocator/Cargo.toml

@@ -16,3 +16,4 @@ rustc_target = { path = "../librustc_target" }
 syntax = { path = "../libsyntax" }
 syntax_pos = { path = "../libsyntax_pos" }
 log = "0.4"
+smallvec = "0.6.4"


llogiq · 2018-08-21T11:05:07Z

src/librustc_data_structures/Cargo.toml

@@ -19,6 +19,7 @@ parking_lot_core = "0.2.8"
 rustc-rayon = "0.1.1"
 rustc-rayon-core = "0.1.1"
 rustc-hash = "1.0.1"
+smallvec = "0.6.4"


llogiq · 2018-08-21T11:05:18Z

src/librustc_mir/Cargo.toml

@@ -25,3 +25,4 @@ syntax = { path = "../libsyntax" }
 syntax_pos = { path = "../libsyntax_pos" }
 byteorder = { version = "1.1", features = ["i128"] }
 rustc_apfloat = { path = "../librustc_apfloat" }
+smallvec = "0.6.4"


llogiq · 2018-08-21T11:06:52Z

I'll push a followup PR to this to change SmallCStr to use SmallVec once this is merged. If this gets merged without the union feature, I can add it then.

gootorov · 2018-08-21T11:21:13Z

Addressed review comments.

Mark-Simulacrum · 2018-08-21T12:03:03Z

@bors try

bors · 2018-08-21T12:03:16Z

⌛ Trying commit 663e279bf28e024a6367226e7de652b8ab4296f1 with merge 40061cbe688a1db38eea0ba1bff9e2a8dc8c8297...

bors · 2018-08-21T13:50:46Z

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

Mark-Simulacrum · 2018-08-21T16:42:59Z

@rust-timer build 40061cbe688a1db38eea0ba1bff9e2a8dc8c8297

rust-timer · 2018-08-21T16:43:00Z

Success: Queued 40061cbe688a1db38eea0ba1bff9e2a8dc8c8297 with parent 70c33bb, comparison URL.

bors · 2018-08-22T22:07:58Z

🔒 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 use-servo-smallvec (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 use-servo-smallvec --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/libsyntax/ext/expand.rs
Auto-merging src/libsyntax/ext/base.rs
Auto-merging src/librustc_traits/chalk_context.rs
Auto-merging src/librustc_resolve/macros.rs
Auto-merging src/librustc_mir/lib.rs
Auto-merging src/librustc_mir/borrow_check/mod.rs
Auto-merging src/librustc/ty/walk.rs
Auto-merging src/librustc/ty/mod.rs
Auto-merging src/librustc/traits/query/normalize.rs
Auto-merging src/librustc/traits/query/dropck_outlives.rs
Auto-merging src/librustc/mir/mod.rs
Auto-merging src/librustc/infer/canonical/mod.rs
Auto-merging src/librustc/infer/canonical/canonicalizer.rs
Auto-merging src/librustc/hir/lowering.rs
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

gootorov · 2018-08-23T07:48:40Z

Rebased and updated smallvec to 0.6.5. The update was a minor version bump - so no code changes and no need to re-review.

oli-obk · 2018-08-23T11:58:56Z

@bors r=Mark-Simulacrum p=1

bors · 2018-08-23T11:58:56Z

📌 Commit 4d81fe9 has been approved by Mark-Simulacrum

michaelwoerister · 2018-08-23T12:04:05Z

@bors r+

bors · 2018-08-23T12:04:06Z

💡 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: Implement try block expressions #52602

bors · 2018-08-23T12:04:07Z

📌 Commit 4d81fe9 has been approved by michaelwoerister

michaelwoerister · 2018-08-23T12:05:07Z

oops... double r+. Looks like it's fine though.

bors · 2018-08-23T13:53:59Z

⌛ Testing commit 4d81fe9 with merge e19ef4419f929459104bcf4a295878ad048bbd6b...

bors · 2018-08-23T14:18:13Z

💔 Test failed - status-travis

rust-highfive · 2018-08-23T14:18:14Z

The job x86_64-gnu-debug 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.

[00:18:43] [RUSTC-TIMING] rustc_incremental test:false 67.682
[00:19:13]    Compiling rustc_allocator v0.0.0 (file:///checkout/src/librustc_allocator)
[00:19:52] [RUSTC-TIMING] rustc_allocator test:false 38.568

Broadcast message from root@travis-job-e31137b6-803c-406d-ae8d-b77ac765799c
 (unknown) at 14:17 ...
The system is going down for power off NOW!
[00:21:17] 
[00:21:17] Session terminated, terminating shell... ...terminated.

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 143.
travis_time:start:0bc5004a
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

michaelwoerister · 2018-08-23T14:35:35Z

@bors retry

bors · 2018-08-23T14:40:31Z

⌛ Testing commit 4d81fe9 with merge e5284b0...

@Mark-Simulacrum

Use optimized SmallVec implementation This PR replaces current SmallVec implementation with the one from the Servo project. Closes #51640 r? @Mark-Simulacrum

bors · 2018-08-23T17:13:33Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing e5284b0 to master...

rust-highfive assigned Mark-Simulacrum Aug 15, 2018

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

gootorov force-pushed the use-servo-smallvec branch from 32a928f to 17b5a97 Compare August 17, 2018 19:46

gootorov force-pushed the use-servo-smallvec branch from 17b5a97 to ea92aa6 Compare August 20, 2018 09:06

llogiq reviewed Aug 21, 2018

View reviewed changes

gootorov force-pushed the use-servo-smallvec branch from ea92aa6 to 663e279 Compare August 21, 2018 11:20

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 22, 2018

Use optimized SmallVec implementation

4d81fe9

gootorov force-pushed the use-servo-smallvec branch from 23264d1 to 4d81fe9 Compare August 23, 2018 07:47

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 23, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 23, 2018

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2018

bors mentioned this pull request Aug 23, 2018

Update Cargo submodule #53567

Merged

bors mentioned this pull request Aug 23, 2018

impl trait in bindings (feature: impl-trait-existential-types) #53542

Merged

bors merged commit 4d81fe9 into rust-lang:master Aug 23, 2018

gootorov deleted the use-servo-smallvec branch August 23, 2018 18:11

Use optimized SmallVec implementation #53384

Use optimized SmallVec implementation #53384

Conversation

gootorov commented Aug 15, 2018

rust-highfive commented Aug 15, 2018

gootorov commented Aug 15, 2018

Mark-Simulacrum commented Aug 15, 2018

bors commented Aug 15, 2018

bors commented Aug 15, 2018

gootorov commented Aug 17, 2018

arthurprs commented Aug 18, 2018

Mark-Simulacrum commented Aug 18, 2018

rust-timer commented Aug 18, 2018

bors commented Aug 19, 2018

gootorov commented Aug 20, 2018

Mark-Simulacrum commented Aug 20, 2018

llogiq Aug 21, 2018

Choose a reason for hiding this comment

llogiq Aug 21, 2018

Choose a reason for hiding this comment

llogiq Aug 21, 2018

Choose a reason for hiding this comment

llogiq Aug 21, 2018

Choose a reason for hiding this comment

llogiq commented Aug 21, 2018 • edited Loading

gootorov commented Aug 21, 2018

Mark-Simulacrum commented Aug 21, 2018

bors commented Aug 21, 2018

bors commented Aug 21, 2018

Mark-Simulacrum commented Aug 21, 2018

rust-timer commented Aug 21, 2018

bors commented Aug 22, 2018

gootorov commented Aug 23, 2018 • edited Loading

oli-obk commented Aug 23, 2018

bors commented Aug 23, 2018

michaelwoerister commented Aug 23, 2018

bors commented Aug 23, 2018

bors commented Aug 23, 2018

michaelwoerister commented Aug 23, 2018

bors commented Aug 23, 2018

bors commented Aug 23, 2018

rust-highfive commented Aug 23, 2018

michaelwoerister commented Aug 23, 2018

bors commented Aug 23, 2018

bors commented Aug 23, 2018

llogiq commented Aug 21, 2018 •

edited

Loading

gootorov commented Aug 23, 2018 •

edited

Loading