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

document FromIterator for Vec allocation behaviors #120355

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Jan 25, 2024

t-libs discussion about #120091 didn't reach a strong consensus, but it was agreed that if we keep the current behavior it should at least be documented even though it is an implementation detail.

The language is intentionally non-committal. The previous (non-existent) documentation permits a lot of implementation leeway and we want retain that. In some cases we even must retain it to be able to rip out some code paths that rely on unstable features.

@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2024

r? @cuviper

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 25, 2024
library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
Comment on lines 2815 to 2818
/// allocations are created, only a small fraction of their items gets collected, no further use
/// is made of the spare capacity and the resulting `Vec` is moved into a longer-lived structure
/// this can lead to the large allocations having their lifetimes unnecessarily extended which
/// can result in increased memory footprint.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a long run-on sentence -- maybe:

Suggested change
/// allocations are created, only a small fraction of their items gets collected, no further use
/// is made of the spare capacity and the resulting `Vec` is moved into a longer-lived structure
/// this can lead to the large allocations having their lifetimes unnecessarily extended which
/// can result in increased memory footprint.
/// allocations are created, and only a small fraction of their items get collected, no further use
/// is made of the spare capacity. If the resulting `Vec` is moved into a longer-lived structure,
/// this can lead to the large allocations having their lifetimes unnecessarily extended which
/// can result in increased memory footprint.

But this scenario about "a large number" of allocations seems a bit too specific to me. It could just as well be one giant vector that gets reduced without any reclaim. I think the point we want to get across is that we may reuse your buffer even if the result has much more capacity that it needs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only do it once then you already paid the max-RSS cost with the temporary allocation. You have to do something like this more than once to blow up max-RSS.

Copy link
Member

@cuviper cuviper Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focusing on RSS assumes an OS that overcommits memory, which isn't universal. And even in that case, all it takes is for the code to fill the "original" vector again to pump up RSS. (edit: nevermind the latter, I mixed that up with the split_off discussion.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you build a giant vector, and then filter-collect it to a much smaller size, you might reasonably expect that giant memory block to now be available for other allocations. This can have an impact even just once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This a long run-on sentence -- maybe

It's supposed to be an "But when a), b) and then c) [happen] this can result X."
The suggested . broke it to "But when a), b). If c) [happens] this can result X."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes that's broken -- that's what I get for not quoting the whole thing when trying to revise it.

library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member Author

the8472 commented Jan 30, 2024

It'd be good if we could get ready to merge within the next week. I have put a link to the to-be-updated nightly documentation in the release notes (#120004) as context.
Or I'll need to find another place to link to.

library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
library/alloc/src/vec/mod.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Jan 30, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 30, 2024

📌 Commit 39dc315 has been approved by cuviper

It is now in the queue for this repository.

@bors 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 Jan 30, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
document `FromIterator for Vec` allocation behaviors

[t-libs discussion](https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202024-01-24/near/417686526) about rust-lang#120091 didn't reach a strong consensus, but it was agreed that if we keep the current behavior it should at least be documented even though it is an implementation detail.

The language is intentionally non-committal. The previous (non-existent) documentation permits a lot of implementation leeway and we want retain that. In some cases we even must retain it to be able to rip out some code paths that rely on unstable features.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#120207 (check `RUST_BOOTSTRAP_CONFIG` in `profile_user_dist` test)
 - rust-lang#120321 (pattern_analysis: cleanup the contexts)
 - rust-lang#120323 (On E0277 be clearer about implicit `Sized` bounds on type params and assoc types)
 - rust-lang#120355 (document `FromIterator for Vec` allocation behaviors)
 - rust-lang#120396 (Account for unbounded type param receiver in suggestions)
 - rust-lang#120430 (std: thread_local::register_dtor fix proposal for FreeBSD.)
 - rust-lang#120435 (Suggest name value cfg when only value is used for check-cfg)
 - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect)
 - rust-lang#120472 (Make duplicate lang items fatal)
 - rust-lang#120490 (Don't hash lints differently to non-lints.)
 - rust-lang#120495 (Remove the `abi_amdgpu_kernel` feature)
 - rust-lang#120501 (rustdoc: Correctly handle attribute merge if this is a glob reexport)

Failed merges:

 - rust-lang#120346 (hir: Refactor getters for owner nodes)

r? `@ghost`
`@rustbot` modify labels: rollup
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Jan 31, 2024
document `FromIterator for Vec` allocation behaviors

[t-libs discussion](https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202024-01-24/near/417686526) about rust-lang#120091 didn't reach a strong consensus, but it was agreed that if we keep the current behavior it should at least be documented even though it is an implementation detail.

The language is intentionally non-committal. The previous (non-existent) documentation permits a lot of implementation leeway and we want retain that. In some cases we even must retain it to be able to rip out some code paths that rely on unstable features.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#120207 (check `RUST_BOOTSTRAP_CONFIG` in `profile_user_dist` test)
 - rust-lang#120321 (pattern_analysis: cleanup the contexts)
 - rust-lang#120355 (document `FromIterator for Vec` allocation behaviors)
 - rust-lang#120430 (std: thread_local::register_dtor fix proposal for FreeBSD.)
 - rust-lang#120469 (Provide more context on derived obligation error primary label)
 - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect)
 - rust-lang#120472 (Make duplicate lang items fatal)
 - rust-lang#120495 (Remove the `abi_amdgpu_kernel` feature)
 - rust-lang#120501 (rustdoc: Correctly handle attribute merge if this is a glob reexport)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#120207 (check `RUST_BOOTSTRAP_CONFIG` in `profile_user_dist` test)
 - rust-lang#120321 (pattern_analysis: cleanup the contexts)
 - rust-lang#120355 (document `FromIterator for Vec` allocation behaviors)
 - rust-lang#120430 (std: thread_local::register_dtor fix proposal for FreeBSD.)
 - rust-lang#120469 (Provide more context on derived obligation error primary label)
 - rust-lang#120472 (Make duplicate lang items fatal)
 - rust-lang#120490 (Don't hash lints differently to non-lints.)
 - rust-lang#120495 (Remove the `abi_amdgpu_kernel` feature)
 - rust-lang#120501 (rustdoc: Correctly handle attribute merge if this is a glob reexport)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 03daaa6 into rust-lang:master Jan 31, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 31, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2024
Rollup merge of rust-lang#120355 - the8472:doc-vec-fromiter, r=cuviper

document `FromIterator for Vec` allocation behaviors

[t-libs discussion](https://rust-lang.zulipchat.com/#narrow/stream/259402-t-libs.2Fmeetings/topic/Meeting.202024-01-24/near/417686526) about rust-lang#120091 didn't reach a strong consensus, but it was agreed that if we keep the current behavior it should at least be documented even though it is an implementation detail.

The language is intentionally non-committal. The previous (non-existent) documentation permits a lot of implementation leeway and we want retain that. In some cases we even must retain it to be able to rip out some code paths that rely on unstable features.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants