Update __rust_[rd]ealloc to take NonNull<u8> instead of *mut u8#153251
Update __rust_[rd]ealloc to take NonNull<u8> instead of *mut u8#153251rust-bors[bot] merged 1 commit intorust-lang:mainfrom
__rust_[rd]ealloc to take NonNull<u8> instead of *mut u8#153251Conversation
Passing null to it is [already UB](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=8dcd25a549c11de72adc94a668277779) anyway.
|
r? @joboet rustbot has assigned @joboet. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| StorageLive(_9); | ||
| _9 = copy _8 as *mut u8 (Transmute); | ||
| _10 = alloc::alloc::__rust_dealloc(move _9, move _5, move _6) -> [return: bb3, unwind unreachable]; | ||
| _9 = alloc::alloc::__rust_dealloc(move _8, move _5, move _6) -> [return: bb3, unwind unreachable]; |
There was a problem hiding this comment.
Notably, this lets us avoid the transmute back from NonNull to *mut.
All the alloc code is doing the NonNull conversion in practice anyway, as it's using https://doc.rust-lang.org/std/alloc/struct.Global.html#method.deallocate not the alloc::dealloc function directly.
|
Makes sense to me. Let's see if this has any effect on performance... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Update `__rust_[rd]ealloc` to take `NonNull<u8>` instead of `*mut u8`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0fd2a86): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.1%, secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -6.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 477.793s -> 479.944s (0.45%) |
|
Yeah, that looks acceptable. |
This comment has been minimized.
This comment has been minimized.
| #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces | ||
| unsafe fn dealloc_nonnull(ptr: NonNull<u8>, layout: Layout) { | ||
| unsafe { __rust_dealloc(ptr, layout.size(), layout.alignment()) } | ||
| } |
There was a problem hiding this comment.
Might be worth it to manually inline these functions. They are tiny and not even encapsulating any unsafeness or tricky logic. Would make optimizations slightly faster and backtraces slightly shorter.
There was a problem hiding this comment.
I would expect that it's already disappeared in the shipped liballoc rlib since the mir inliner should have done that for us already. So it would remove a inlined entry in the mir scopes, but that's it, afaict.
There was a problem hiding this comment.
MIR inlined calls do still show in backtraces.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing ddd36bd (parent) -> 8d50bcc (this PR) Test differencesShow 11 test diffs11 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8d50bccc5bd70be9f5a2fc98c0857e24b3dcdf85 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (8d50bcc): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.6%, secondary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.0%, secondary -1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 478.623s -> 479.657s (0.22%) |

Passing null to it is already UB per Miri anyway, and this is ABI-compatible as
NonNullisrepr(transparent).Inspired by #152702 (comment) ; Similar to #152605 which changed
align: usizetoalign: ptr::Alignment.