document FromIterator for Vec allocation behaviors
#120355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
cuviper
reviewed
Jan 26, 2024
library/alloc/src/vec/mod.rs
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
Hmm, yes that's broken -- that's what I get for not quoting the whole thing when trying to revise it.
Kobzol
reviewed
Jan 26, 2024
the8472
force-pushed
the
doc-vec-fromiter
branch
from
29d8d40
to
57009e1
Compare
kadiwa4
reviewed
Jan 28, 2024
the8472
force-pushed
the
doc-vec-fromiter
branch
from
3be4a96
to
8888bfc
Compare
|
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. |
cuviper
reviewed
Jan 30, 2024
Co-authored-by: Josh Stone <[email protected]>
the8472
force-pushed
the
doc-vec-fromiter
branch
from
2cddfa5
to
39dc315
Compare
|
@bors r rollup |
bors
added
S-waiting-on-bors
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/286302-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/286302-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
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/286302-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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.