-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add Vec visualization to understand capacity #76066
Conversation
Visualize vector while differentiating between stack and heap.
cc @jyn514 @GuillaumeGomez Do you think it look nice? Weird it didn't assign anyone |
cc @rust-lang/docs |
See also: |
cc @rust-lang/docs Oh, it doesn't work for me. @GuillaumeGomez have link but I don't have. T_T |
Hmm ... maybe this belongs in the book instead, in the section on stack and heap? https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html Anyways I don't think I'm the best person to review this, maybe @steveklabnik would be interested? |
Co-authored-by: Joshua Nelson <[email protected]>
We could put it there but we also mention about |
I am still worried that this exposes details that are not stable. |
So the ascii chart should make clear that it is only an example of a possible implementation, not the real one. And it probably should show an incorrect layout just to make sure that anyone who even tries to rely on it will immediately run into a problem. |
Yes, it should mention that ABI is not stable yet.
Incorrect layout? |
It should give an order of fields that does not match the current implementation. |
For example? |
E.g. put the pointer last. In the current implementation (assuming no reordering by the compiler) it is the first element of the struct. |
@the8472 I already mentioned that ABI is not stable so layout is not guaranteed. |
@@ -194,6 +194,22 @@ use crate::raw_vec::RawVec; | |||
/// can be slow. For this reason, it is recommended to use [`Vec::with_capacity`] | |||
/// whenever possible to specify how big the vector is expected to get. | |||
/// | |||
/// In summary, a vector containing 1, 2 with capacity 4 can be visualized as: | |||
/// ```text | |||
/// Stack ptr capacity len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just remove "Stack". We simply don't know where those three values live. We just know that the lower box lives on the heap.
Co-authored-by: Lukas Kalbertodt <[email protected]>
Triage: What's the current status on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crlf0710 this needs a new reviewer, I think Lukas has left the libs team.
@@ -194,6 +194,22 @@ use crate::raw_vec::RawVec; | |||
/// can be slow. For this reason, it is recommended to use [`Vec::with_capacity`] | |||
/// whenever possible to specify how big the vector is expected to get. | |||
/// | |||
/// In summary, a vector containing 1, 2 with capacity 4 can be visualized as: | |||
/// ```text | |||
/// Stack ptr capacity len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try 'Layout' instead, maybe?
Lucas suggested something too, maybe we should wait for the new reviewer to say. Feel like this is something that can be bikeshed on, I personally would use 'Heap'. |
ping from triage: @pickfire can you please resolve the merge conflict? thank you. |
@JohnCSimon Fix conflict |
CI is now failing unfortunately, you probably need to fix tidy. Also it would be nice to squash the commits. |
r? @m-ou-se |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a useful addition!
A bit later in the text, in the "Guarantees" section, this layout is explained in words. Maybe this visualisation should be put there instead? (Right after the sentence ending in logically uninitialized, contiguous elements.
)
(It's a bit of a shame that this has to use +---+
to draw the boxes, but none of the alternatives (an svg, raw html, a markdown table, or unicode box drawing characters) work well. I couldn't find something better that is usable and reliable in both the browser and terminal.)
@@ -194,6 +194,22 @@ use crate::raw_vec::RawVec; | |||
/// can be slow. For this reason, it is recommended to use [`Vec::with_capacity`] | |||
/// whenever possible to specify how big the vector is expected to get. | |||
/// | |||
/// In summary, a vector containing 1, 2 with capacity 4 can be visualized as: | |||
/// ```text | |||
/// Stack ptr capacity len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like "The Vec object itself" and "The data on the heap".
/// ```text | ||
/// Stack ptr capacity len | ||
/// +--------+--------+--------+ | ||
/// | 0x0123 | 4 | 2 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
into_raw_parts
and from_raw_parts
use (ptr, length, capacity)
, not (ptr, capacity, length)
. It might be good to use the same order here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentionally different, see #76066 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyn514 That comment suggests to use a different layout than the real layout. The 'real' layout is (RawVec, len), which is effectively a (ptr, cap, len). Changing it to (ptr, len, cap) as I suggest does make it different from the actual layout, just like the8472 asked for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I didn't notice this, I thought I used len
followed by capacity
.
/// - Note: the ABI is not stable and `Vec` makes no guarantees about its memory | ||
/// layout (including the order of fields). See [the section about | ||
/// guarantees](#guarantees). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout of the part on the heap is pretty strict. Let's make clear it's only the Vec object itself (the (ptr, size, cap) part) that's unspecified. A mention of into_raw_parts
/from_raw_parts
and/or .as_ptr()
/.len()
/.capacity()
would also be nice here, as those are the correct way of interacting with these fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can suggest a change here?
Yes, I am thinking of creating a new PR for this.
What? |
Note that you can force push to the same branch after sqashing in order to reuse the same PR, it's not technically necessary to start a new PR. Opening a new PR makes it harder to follow the discussion as one needs to jump between PRs now and potentially requires verbatim copying comments from the old PR. |
Excuse me?
There's plenty of space to put more words, if that makes things clearer.
The elements of the vector (the second part of your diagram) are on the heap. Only the Vec object itself can be on the stack.
Next time please just force-push to the same branch if you need to rebase or squash, otherwise it's hard to keep track of the review comments. |
Yes, but it is more troublesome, the branch name is also weird.
No, I still want to see the condition first, in this case it would be easier to create a new pull request. But if other case where I already have a branch I will force-push. |
To locally change a pull request you opened using the github api you can use git fetch remote-name
git checkout remote-name/patch-15 -b patch-15 Splitting the conversation over two pull requests, especially if there are some open threads on the old one, has a noticeable negative impact on the review process. So while it might be a bit easier for you to open a new PR, I strongly advise you to use the commands above in the future and keep everything in one place. By doing so you prevent potential oversights without wasting valuable time of both the reviewers and yourself. |
Visualize vector while differentiating between stack and heap. Inspired by cheats.rs, as this is probably the first place beginner go, they could understand stack and heap, length and capacity with this. Not sure if adding this means we should add to other places too. Superseeds rust-lang#76066
Add Vec visualization to understand capacity Visualize vector while differentiating between stack and heap. Inspired by cheats.rs, as this is probably the first place beginner go, they could understand stack and heap, length and capacity with this. Not sure if adding this means we should add to other places too. Superseeds rust-lang#76066 r? `@m-ou-se` cc `@the8472` I put back the order of the fields as it feels weird, the note already explains that the order of fields is not guaranteed
Visualize vector while differentiating between stack and heap.
Inspired by cheats.rs, this is probably the first place beginner go, they could understand stack and stack, length and capacity with this. Not sure if adding this means we should add to other places too.
r? @rust-lang/rustdoc