Fallback when allocating a new chunk fails

[Kerollmops]

When the global allocator is unable to allocate a new chunk, mostly due to the size, the Bump struct was either panicking or returning an AllocErr. This PR, discussed in a comment, changes the policy of the Bump::alloc_layout_slow:

1. Try to allocate a chunk twice the size of the last one,
2. If rejected, try with a chunk of the same size as the last one,
3. If rejected, try with a chunk of half the size of the last one,
4. If rejected, continue trying with a size half the previously tried one,
5. If the size tried is smaller than the default chunk size, return None instead of the footer, the same behavior as before.

Trying to allocate half the size of the last chunk helps in allocating the whole available memory, it creates smaller allocations that will more easily fit in the available global memory.

[Kerollmops]

Not sure about this change, what do you think? Do I need to move it to the alloc_layout_slow method?

[fitzgen]

Can you instead debug_assert! the property that the new chunk can hold the requested_layout if any?

[Kerollmops]

I tried another allocating policy, where it always try to allocate half the previously tried allocated size, starting from twice the size of the last allocated chunk and stopping when the size is smaller than the default chunk size. I implemented this because I again faced an allocation error due to the fact that the remaining memory what a little bit less than half the size of the last allocated chunk size (the current policy of this PR).

[fitzgen]

We can't have this behavior in new_chunk directly because that will break with_capacity's and try_with_capacity's guarantees that if it successfully returns then you have that much given capacity.

I think the way to go is by defining a new function that wraps new_chunk in a loop and tries to allocate double the current chunk, then the current chunk, then half the current chunk, then a quarter, etc... and fails if allocating the default chunk size fails or if it can't allocate something large enough for the requested allocation's layout.

Does that make sense?

[Kerollmops]

It totally makes sense and this is why I didn't modify the new_chunk method a lot, I only changed the first parameter to be the expected chunk size instead of the old chunk size, this way it is easier to know what the new_chunk method will allocate. It doesn't impact the try_/with_capacity methods as they do not use this first parameter.

However, I modified the alloc_layout_slow method to implement the algorithm that you described in your comment.

[fitzgen]

Thanks @Kerollmops! This looks good to me, modulo the little suggestions below. After those are addressed, I'll be happy to merge this! Thanks again!

[fitzgen]

Can you instead debug_assert! the property that the new chunk can hold the requested_layout if any?

[fitzgen]

Suggested change

	if base_size >= DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER {
	if base_size >= DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER && base_size >= layout.size {

This isn't technically 100% necessary for correctness, since Bump::new_chunk will round up to the requested layout's size/align. However, consider the scenario where the requested size is very large and the global allocator can't fulfill it: if we didn't have this additional check, we would ask for smaller and smaller chunks, but new_chunk would have to round those chunk sizes up to the exact same requested layout size every time, effectively making a pretty-much-known-to-fail malloc call log n times.

[fitzgen]

We could also combine the checks with something like this:

let min_new_chunk_size = layout.size.max(DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER);
let sizes = iter::from_fn(|| {
    if base_size >= min_new_chunk_size {
        // ...
    } else {
        // ...
    }
});

[fitzgen]

Teensy tiny nitpick over wording.

Suggested change

	// As a sane default, we want our new allocation to be about twice as
	// big as the previous allocation. If the global allocator refuses it,
	// By default, we want our new chunk to be about twice as
	// big as the previous chunk. If the global allocator refuses it,

[fitzgen]

Looks great! Thanks for your patience with the multiple rounds of review.

[fitzgen]

Looks like there are errors in CI:

https://github.com/fitzgen/bumpalo/pull/111/checks?check_run_id=2667680337#step:6:246

Can you reproduce this locally?

[Kerollmops]

Hey, yes, I just fixed it, what do you think?

Looks great! Thanks for your patience with the multiple rounds of review.

No worries, it is completely understandable 😃

[fitzgen]

Thanks!!