-
Notifications
You must be signed in to change notification settings - Fork 13.4k
rustdoc: remove ItemInner
.
#138916
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
rustdoc: remove ItemInner
.
#138916
Conversation
The `Item` struct is 48 bytes and contains a `Box<ItemInner>`; `ItemInner` is 104 bytes. This is an odd arrangement. Normally you'd have one of the following. - A single large struct, which avoids the allocation for the `Box`, but can result in lots of wasted space in unused parts of a container like `Vec<Item>`, `HashSet<Item>`, etc. - Or, something like struct `Item(Box<ItemInner>)`, which requires the `Box` allocation but gives a very small `Item` size, which is good for containers like `Vec<Item>`. (`Vec<Box<Item>>` would also work.) `Item`/`ItemInner` currently gets the worst of both worlds: it always requires a `Box`, but `Item` is also pretty big and so wastes space in containers. It would make sense to push it in one direction or the other. This commit does the first option, a single large struct.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustdoc: remove `ItemInner`. The `Item` struct is 48 bytes and contains a `Box<ItemInner>`; `ItemInner` is 104 bytes. This is an odd arrangement. Normally you'd have one of the following. - A single large struct, which avoids the allocation for the `Box`, but can result in lots of wasted space in unused parts of a container like `Vec<Item>`, `HashSet<Item>`, etc. - Or, something like struct `Item(Box<ItemInner>)`, which requires the `Box` allocation but gives a very small `Item` size, which is good for containers like `Vec<Item>`. (`Vec<Box<Item>>` would also work.) `Item`/`ItemInner` currently gets the worst of both worlds: it always requires a `Box`, but `Item` is also pretty big and so wastes space in containers. It would make sense to push it in one direction or the other. This commit does the first option, a single large struct. r? `@ghost`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (fd7707c): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.0%, secondary 1.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.153s -> 777.694s (0.07%) |
The `Item` struct is 48 bytes and contains a `Box<ItemInner>`; `ItemInner` is 104 bytes. This is an odd arrangement. Normally you'd have one of the following. - A single large struct, which avoids the allocation for the `Box`, but can result in lots of wasted space in unused parts of a container like `Vec<Item>`, `HashSet<Item>`, etc. - Or, something like `struct Item(Box<ItemInner>)`, which requires the `Box` allocation but gives a very small Item size, which is good for containers like `Vec<Item>`. `Item`/`ItemInner` currently gets the worst of both worlds: it always requires a `Box`, but `Item` is also pretty big and so wastes space in containers. It would make sense to push it in one direction or the other. rust-lang#138916 showed that the first option is a regression for rustdoc, so this commit does the second option, which improves speed and reduces memory usage.
This was a clear regression for icounts, cycles, wall-time, and max-rss. #138927 takes the other approach and improves perf. |
…, r=GuillaumeGomez rustdoc: Rearrange `Item`/`ItemInner`. The `Item` struct is 48 bytes and contains a `Box<ItemInner>`; `ItemInner` is 104 bytes. This is an odd arrangement. Normally you'd have one of the following. - A single large struct, which avoids the allocation for the `Box`, but can result in lots of wasted space in unused parts of a container like `Vec<Item>`, `HashSet<Item>`, etc. - Or, something like `struct Item(Box<ItemInner>)`, which requires the `Box` allocation but gives a very small Item size, which is good for containers like `Vec<Item>`. `Item`/`ItemInner` currently gets the worst of both worlds: it always requires a `Box`, but `Item` is also pretty big and so wastes space in containers. It would make sense to push it in one direction or the other. rust-lang#138916 showed that the first option is a regression for rustdoc, so this commit does the second option, which improves speed and reduces memory usage. r? `@GuillaumeGomez`
The
Item
struct is 48 bytes and contains aBox<ItemInner>
;ItemInner
is 104 bytes. This is an odd arrangement. Normally you'd have one of the following.A single large struct, which avoids the allocation for the
Box
, but can result in lots of wasted space in unused parts of a container likeVec<Item>
,HashSet<Item>
, etc.Or, something like struct
Item(Box<ItemInner>)
, which requires theBox
allocation but gives a very smallItem
size, which is good for containers likeVec<Item>
. (Vec<Box<Item>>
would also work.)Item
/ItemInner
currently gets the worst of both worlds: it always requires aBox
, butItem
is also pretty big and so wastes space in containers. It would make sense to push it in one direction or the other. This commit does the first option, a single large struct.r? @ghost