Skip to content
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

Try to guess a smarter initial capacity in Vec::from_iter #53086

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@scottmcm
Copy link
Member

scottmcm commented Aug 5, 2018

Another possibility is collect could look at the upper bound and be smarter about what capacity to use? ~ #45840 (comment)

This is obviously good for hints like (60, Some(61)) where today we allocate space for 60, then double it should the additional element show up, and it'd be much better to just always allocate 61.

More nuanced are hints like (0, Some(150)), where today the code uses just the 0, and thus starts at a capacity of 1, but with this change will start at 10 instead.

This can undeniably increase memory pressure over where it is today, so I expect at least some controversy 🙂 It does use try_reserve for the allocation that's more than the lower bound, and thus shouldn't introduce new aborts, at least. And the starting point grows by the root, keeping things fairly contained: even with an hint of (0, Some(1_000_000_000)) it'll only start at 30_517.

cc @ljedrz
cc #48994

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 5, 2018

r? @kennytm

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

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Aug 5, 2018

This is potentially awesome not only for Option/Result::FromIterator, but also for all interators with a lower bound of zero (filters etc.). I would love to see a perf run.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 5, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 5, 2018

⌛️ Trying commit fe30b5b with merge 8612aa2...

bors added a commit that referenced this pull request Aug 5, 2018

Auto merge of #53086 - scottmcm:use-upper-in-collect, r=<try>
Try to guess a smarter initial capacity in Vec::from_iter

> Another possibility is collect could look at the upper bound and be smarter about what capacity to use?  ~ #45840 (comment)

This is obviously good for hints like `(60, Some(61))` where today we allocate space for 60, then double it should the additional element show up, and it'd be much better to just always allocate 61.

More nuanced are hints like `(0, Some(150))`, where today the code uses just the `0`, and thus starts at a capacity of `1`, but with this change will start at `10` instead.

This can undeniably increase memory pressure over where it is today, so I expect at least some controversy 🙂  It does use `try_reserve` for the allocation that's more than the lower bound, and thus shouldn't introduce new aborts, at least.  And the starting point grows by the root, keeping things fairly contained: even with an hint of `(0, Some(1_000_000_000))` it'll only start at `30_517`.

cc @ljedrz
cc #48994
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 5, 2018

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

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 5, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Aug 5, 2018

Success: Queued 8612aa2 with parent b47c314, comparison URL.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 6, 2018

Perf is ready.

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Aug 6, 2018

Some greens, some reds... I'd say the results are inconclusive 🤔.

@scottmcm

This comment has been minimized.

Copy link
Member Author

scottmcm commented Aug 6, 2018

Hmm, for instructions things seem slightly worse overall :(

How reliable are the time measurements? It's interesting that webrender-clean-opt is +0.3% for instructions:u, but -1.5% for wall-time.

(Another thing that surprised me: max-rss -3.5% for webrender-debug-clean.)

I think I'll try something less complicated than the sqrt approximation for the guess; maybe it's just not worth doing that work and making LLVM compile it when people use Vec, even though it's simple integer instructions. (It's less and less helpful the larger the gap anyway.)

scottmcm added some commits Aug 5, 2018

Be much less clever about things
This one only helps lower>0, but by doing so means it's always strictly less than 100% overhead, same as the normal doubling algorithm.  And thus doesn't need to do the try_reserve dance or a post-extend cleanup.

@scottmcm scottmcm force-pushed the scottmcm:use-upper-in-collect branch from fe30b5b to aa080f4 Aug 8, 2018

@scottmcm

This comment has been minimized.

Copy link
Member Author

scottmcm commented Aug 8, 2018

Ok, I've pushed a much simpler change that's only targeted at one side of the problem. Could I get another try+perf run to see whether it's fruitful overall, please?

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 8, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 8, 2018

⌛️ Trying commit aa080f4 with merge 68f1266...

bors added a commit that referenced this pull request Aug 8, 2018

Auto merge of #53086 - scottmcm:use-upper-in-collect, r=<try>
Try to guess a smarter initial capacity in Vec::from_iter

> Another possibility is collect could look at the upper bound and be smarter about what capacity to use?  ~ #45840 (comment)

This is obviously good for hints like `(60, Some(61))` where today we allocate space for 60, then double it should the additional element show up, and it'd be much better to just always allocate 61.

More nuanced are hints like `(0, Some(150))`, where today the code uses just the `0`, and thus starts at a capacity of `1`, but with this change will start at `10` instead.

This can undeniably increase memory pressure over where it is today, so I expect at least some controversy 🙂  It does use `try_reserve` for the allocation that's more than the lower bound, and thus shouldn't introduce new aborts, at least.  And the starting point grows by the root, keeping things fairly contained: even with an hint of `(0, Some(1_000_000_000))` it'll only start at `30_517`.

cc @ljedrz
cc #48994
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 8, 2018

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

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 8, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Aug 8, 2018

Success: Queued 68f1266 with parent 26d7b64, comparison URL.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 9, 2018

Perf is ready.

@scottmcm

This comment has been minimized.

Copy link
Member Author

scottmcm commented Aug 12, 2018

Hmm, that's again a bit too mixed for me to be happy about.

I'll just close this. Anyone interested should feel free to pick this space up again; feel free to use or ignore anything I did in this PR 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.