Remove OneVector type alias by ljedrz · Pull Request #53824 · rust-lang/rust

ljedrz · 2018-08-30T14:34:45Z

Removes the OneVector type alias (equivalent to SmallVec<[T; 1]>); it is used in scenarios where the capacity of 1 is often exceeded, which might be nullifying the performance wins (due to spilling to the heap) expected when using SmallVec instead of Vec.

The numbers I used in this PR are very rough estimates - it would probably be a good idea to adjust some/all of them, which is what this proposal is all about.

It might be a good idea to additionally create some local type aliases for the SmallVecs in the Folder trait, as they are repeated in quite a few spots; I'd be happy to apply this sort of adjustments.

rust-highfive · 2018-08-30T14:34:50Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

bors · 2018-08-31T05:59:03Z

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

llogiq · 2018-08-31T10:50:48Z

FYI: I''ve started writing a smallvectune crate that can wrap SmallVec and log capacity information by internal array size over the course of the various constructors, resizings and deallocations. There are still some things to do, but once it's working, we could use this to help us gauge optimal internal array sizes for some workloads.

ljedrz · 2018-09-11T13:21:35Z

Rebased.

eddyb · 2018-09-11T19:53:12Z

r? @nnethercote cc @nikomatsakis @michaelwoerister

nnethercote · 2018-09-11T23:48:56Z

This PR currently makes two changes:

remove the OneVector type alias (and ExpectOne);
expand almost all of the formerly OneVector small vecs from 1 inline entry to 4 (and one in transcribe.rs to 8).

The first change seems fine. I agree that OneVector is not a particularly good name. You also mention the possibility of adding new type aliases. SmallVec1, SmallVec4, etc., would be clearer, but I'm not sure if they are necessary.

I'm less satisfied by the second change. The description above says "it is used in scenarios where the capacity of 1 is often exceeded, which might be nullifying the performance wins" -- do you have any measurements showing this? My general approach is profile-driven: I assume the existing code is reasonable, and only change such things when I have measurements showing it helps. I'm not comfortable with a blanket approach of changing every OneVector to have 4 inline entries without measurements. Avoiding heap allocation is good in general, but SmallVec can cause extra memcpy calls and I have seen cases where the cost of the additional copying exceeding the savings from the avoided heap allocations. It could also increase memory usage.

To move forward I would suggest removing OneVector and changing all its uses to SmallVector<[T; 1]> is fine. Beyond that, I think it's only worth changing SmallVec lengths when measurements show it helps. @llogiq's smallvectune crate could be very useful for that.

nnethercote

(Changes requested are above.)

nnethercote · 2018-09-11T23:52:09Z

This is the one that is changed to 8, BTW.

ljedrz · 2018-09-12T11:15:01Z

@nnethercote Thanks for the review! I wasn't aware these values were profiled and I now understand that changing them in an ad-hoc manner could be just as bad as spilling to the heap.

I agree that waiting for a more precise solution is the right choice. The adjusted PR should make this future change easier.

nnethercote · 2018-09-12T22:03:34Z

I wasn't aware these values were profiled and I now understand that changing them in an ad-hoc manner could be just as bad as spilling to the heap.

Well, it's possible they are profiled, I don't know for sure. You could work it out with git blame if you really wanted. It's also possible that the exact size doesn't matter for a lot of them. But in the absence of measurements, I think it's best to leave things as they are. This is basically Chesterton's Fence in action:

There exists in such a case a certain institution or law; let us say, for the sake of simplicity, a fence or gate erected across a road. The more modern type of reformer goes gaily up to it and says, "I don't see the use of this; let us clear it away." To which the more intelligent type of reformer will do well to answer: "If you don't see the use of it, I certainly won't let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it."

In this case measurements are the equivalent of the "Go away and think" part.

llogiq · 2018-09-13T06:32:42Z

smallvectune has reached its first milestone: The data collector should work now, and uses an MPSC to collect the traces, so it's all thread-safe and shouldn't be too much of a performance burden (that said, I haven't benchmarked it...).

bors · 2018-09-17T21:18:19Z

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

TimNN · 2018-09-25T17:37:25Z

Ping from triage! @nnethercote / @ljedrz: Could you confirm the status of this PR? IIUC, it now only removes a type alias and uses a SmallVec implementation from an external crate.

nnethercote · 2018-09-25T20:29:28Z

As per #53824 (comment), I suggest that @ljedrz changes this PR to just be about removing OneVector, and then does size tuning in a follow-up.

ljedrz · 2018-09-26T07:56:14Z

@nnethercote actually I already did that. I'll rebase it, this change should make future size tuning easier.

ljedrz · 2018-09-26T08:45:14Z

Rebased.

michaelwoerister · 2018-09-26T09:03:57Z

Looking good!
@bors r+

bors · 2018-09-26T09:03:58Z

📌 Commit 130a32f has been approved by michaelwoerister

bors · 2018-09-26T10:04:48Z

⌛ Testing commit 130a32f with merge c3a1a0d...

Remove OneVector, increase related SmallVec capacities Removes the `OneVector` type alias (equivalent to `SmallVec<[T; 1]>`); it is used in scenarios where the capacity of 1 is often exceeded, which might be nullifying the performance wins (due to spilling to the heap) expected when using `SmallVec` instead of `Vec`. The numbers I used in this PR are very rough estimates - it would probably be a good idea to adjust some/all of them, which is what this proposal is all about. It might be a good idea to additionally create some local type aliases for the `SmallVec`s in the `Folder` trait, as they are repeated in quite a few spots; I'd be happy to apply this sort of adjustments.

bors · 2018-09-26T12:39:14Z

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

rust-highfive · 2018-09-26T12:40:39Z

📣 Toolstate changed by #53824!

Tested on commit c3a1a0d.
Direct link to PR: #53824

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@c3a1a0d. Direct link to PR: <rust-lang/rust#53824> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

fix breakage by rust-lang/rust#53824 use smallvec crate instead of rustcs type alias.

gnzlbg · 2018-10-02T09:47:45Z

This broke interpolate idents.

rust-highfive assigned eddyb Aug 30, 2018

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

This comment has been minimized.
Sign in to view

ljedrz force-pushed the begone_onevector branch from eff2d5c to 6f3bbc0Compare August 30, 2018 17:16

This comment has been minimized.
Sign in to view

ljedrz force-pushed the begone_onevector branch from 6f3bbc0 to 29fd8a4Compare August 30, 2018 19:29

ljedrz force-pushed the begone_onevector branch from 29fd8a4 to 35ea50cCompare September 11, 2018 13:20

nnethercote requested changes Sep 11, 2018
View reviewed changes

Comment threadsrc/libsyntax/ext/tt/transcribe.rs Outdated
nnethercoteSep 11, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one that is changed to 8, BTW.

scottmcm 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 Sep 12, 2018

ljedrz force-pushed the begone_onevector branch from 35ea50c to d04a2a6Compare September 12, 2018 11:08

Remove OneVector
130a32f

ljedrz force-pushed the begone_onevector branch from d04a2a6 to 130a32fCompare September 26, 2018 08:45

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 Sep 26, 2018

bors merged commit 130a32f into rust-lang:masterSep 26, 2018

ljedrz deleted the begone_onevector branch September 26, 2018 12:41

ljedrz changed the title ~~Remove OneVector, increase related SmallVec capacities~~Remove OneVector type aliasSep 26, 2018

This was referenced Sep 26, 2018
Added graphviz visualization for obligation forests. #54486
Merged
resolve: Some refactorings in preparation for uniform paths 2.0 #54336
Merged

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Sep 26, 2018
rustup
fc35c20
fix breakage by rust-lang/rust#53824 use smallvec crate instead of rustcs type alias.

This was referenced Sep 26, 2018
rustup rust-lang/rust-clippy#3226
Merged
update clippy submodule to a72e786c #54561
Merged

dtolnay mentioned this pull request Sep 27, 2018
OneVector type alias has been removed dtolnay/syn#504
Merged

gnzlbg mentioned this pull request Oct 2, 2018
OneVector has been removed SkylerLipthay/interpolate_idents#42
Closed

pnkfelix mentioned this pull request Nov 12, 2018
Lifetimes bug on beta 1.31: the associated type may not live long enough #55756
Closed

Uh oh!

Uh oh!

Conversation

ljedrz commented Aug 30, 2018

Uh oh!

rust-highfive commented Aug 30, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

bors commented Aug 31, 2018

Uh oh!

llogiq commented Aug 31, 2018

Uh oh!

ljedrz commented Sep 11, 2018

Uh oh!

eddyb commented Sep 11, 2018

Uh oh!

nnethercote commented Sep 11, 2018

Uh oh!

nnethercote left a comment

Choose a reason for hiding this comment

Uh oh!

nnethercoteSep 11, 2018

Choose a reason for hiding this comment

Uh oh!

ljedrz commented Sep 12, 2018

Uh oh!

nnethercote commented Sep 12, 2018

Uh oh!

llogiq commented Sep 13, 2018

Uh oh!

bors commented Sep 17, 2018

Uh oh!

TimNN commented Sep 25, 2018

Uh oh!

nnethercote commented Sep 25, 2018

Uh oh!

ljedrz commented Sep 26, 2018

Uh oh!

ljedrz commented Sep 26, 2018

Uh oh!

michaelwoerister commented Sep 26, 2018

Uh oh!

bors commented Sep 26, 2018

Uh oh!

bors commented Sep 26, 2018

Uh oh!

bors commented Sep 26, 2018

Uh oh!

rust-highfive commented Sep 26, 2018

Uh oh!

gnzlbg commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants