Remove fast path in reallocation for same layout sizes #75621
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Typo nit: -// If `new_size` is zero, than `old_size` has '…
+// If `new_size` is zero, then `old_size` has '… |
|
Thanks :) |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit c619b36 with merge da66a9e07441aed971b868d823aef30f0be216e7... |
|
Is homu broken? The queue does not show this PR. |
|
@bors try |
|
⌛ Trying commit c48f784 with merge 4e9d2055704b5bcf1e6dae96a5145640b3bab32c... |
|
It does show it for me. |
|
I think it gets canceled if you push to the branch. |
|
Didn't noticed you have approved before I have corrected the typo. |
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued 4e9d2055704b5bcf1e6dae96a5145640b3bab32c with parent 8cdc94e, future comparison URL. |
|
Finished benchmarking try commit (4e9d2055704b5bcf1e6dae96a5145640b3bab32c): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
Seems like a minor speed boost (up to 0.6%). @bors r+ |
|
📌 Commit c48f784 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c48f784 with merge edd08637821fc8e3860b1f6057ac394a9b638532... |
|
@bors retry |
| @@ -209,16 +209,14 @@ unsafe impl AllocRef for Global { | |||
| ); | |||
|
|
|||
| // SAFETY: `new_size` must be non-zero, which is checked in the match expression. | |||
| // If `new_size` is zero, then `old_size` has to be zero as well. | |||
| // Other conditions must be upheld by the caller | |||
There was a problem hiding this comment.
This is not documented though, is it? It least grow does not have a comment explaining the caller obligations.
Looks like new_size >= old_size would have to be among those.
There was a problem hiding this comment.
It's a safety constraint, that new_size must be greater than or equal to old_size. If old_size is non-zero (was checked just before), new_size can't be zero.
There was a problem hiding this comment.
I have a commit ready to be pushed, which cleans up the implementations and documentation of AllocRef a bit, so those things get more clear. The unsafe should be moved to line 217 instead. The match arm, where layout.size() == 0 does not need unsafe at all.
There was a problem hiding this comment.
Right, my only comment is that this is an undocumented safety constraint -- at least when looking just at the grow function, and not at the entire file.
EDIT: well, there is a debug assertion. But the usual comment explaining to the caller what they have to uphold is missing.
There was a problem hiding this comment.
However, I didn't want to change implementations much in this PR because of the perf-run.
There was a problem hiding this comment.
But the usual comment explaining to the caller what they have to uphold is missing.
This is stated in the safety-section of the trait.
There was a problem hiding this comment.
Oh I see... in GH I couldn't see that this is a trait impl. Sorry.
There was a problem hiding this comment.
No Problem!
In any case #75657 should make this more readable if you only read the code. :)
|
☀️ Test successful - checks-actions, checks-azure |
r? @Amanieu
Before merging a perf-run should be done.
Closes rust-lang/wg-allocators#70