Improve liballoc_jemalloc {re}alloc_excess

[arthurprs]

It should now behave as expected.

cc #45434

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[gnzlbg]

I did it like this at first in #45512 but I think this is wrong. nallocx returns the total allocation size, so excess should be that - size. #45512 does it like that.

[arthurprs]

My understanding is that excess (despite the name) should have the entire capacity, not the extra.

    unsafe fn realloc_excess(&mut self,
                             ptr: *mut u8,
                             layout: Layout,
                             new_layout: Layout) -> Result<Excess, AllocErr> {
        let mut err = ManuallyDrop::new(mem::uninitialized::<AllocErr>());
        let mut size = 0;
        let ptr = __rust_realloc_excess(ptr,
                                        layout.size(),
                                        layout.align(),
                                        new_layout.size(),
                                        new_layout.align(),
                                        &mut size,
                                        &mut *err as *mut AllocErr as *mut u8);
        if ptr.is_null() {
            Err(ManuallyDrop::into_inner(err))
        } else {
            Ok(Excess(ptr, size))
        }
    }

[gnzlbg]

I just checked the docs of Excess and you are correct, the usize there is the total capacity.

[gnzlbg]

I just commented on the #32838 tracking issue about this, I found it really confusing. Not only in the Allocator trait, but also because it propagates back to the wrappers over the allocator functions like here.

[arthurprs]

Yes, the classic naming problem.

[gnzlbg]

I also think this is wrong: nallocx returns the excess size of mallocx but you are using rallocx to allocate (and it doesn't return the excess size but the total size).

You can either use sdallocx(ptr, align) to get the real size allocated for pointer (which I think is more expensive than nallocx), or do like #45514 and use xallocx to allocate instead of rallocx which gives you the total size back.

[arthurprs]

Good point about salloc, I'll change the code to use that.

xallocx isn't general, it can "fail to grow" if the reallocation can't be done inplace, see https://github.com/jemalloc/jemalloc/blob/47203d5f422452def4cb29c0b7128cc068031100/test/integration/xallocx.c#L51

[gnzlbg]

xallocx isn't general, it can "fail to grow" if the reallocation can't be done inplace

Indeed, I've asked about this in jemalloc's issues earlier today: jemalloc/jemalloc#1059

Ideally what we would like to use is nallocx equivalent for rallocx, or a rallocx variant that returns the excess size. My understanding is that sdallocx is correct for all cases, but it can be expensive because it might traverse jemalloc internal data-structures to find the memory.

[gnzlbg]

Modulo the issues mentioned I think we should make this change.

I've filled it in two different pull-requests (#45512, #45514) because I want to run perf on both independently since that will tell us more about the impact of nallocx and xallocx on performance which is useful to know while optimizing RawVec.

Did you manage to find any information about why these changes weren't implemented before? I've been trying to find anything about this in the issue tracker without luck.

[gnzlbg]

Note that currently sallocx is not provided.

[gnzlbg]

Once this is done could you split this into two independent pull-request so that we can run perf on both ? That way I can close mine.

[arthurprs]

Why split PRs though? It's a 30 line change.

[gnzlbg]

nvm, I just wanted to get perf result for the two independent changes mostly because I don't know the cost of the sallocx call, might be as expensive as a rallocx call. But I have the changes split in both PRs already so I can ask for perf runs on those. For merging purposes a single PR is fine.

[arthurprs]

I see what you meant now. I'll go ahead and close this one so you can have more control of what's tested.