Add support for allocation limits on arenas #160

Eliasin · 2022-06-02T02:10:09Z

Implements #135

Arenas can optionally keep track of an allocation_limit which is
considered when allocating new chunks to an arena.

The limit is only checked in alloc_layout_slow and new_chunk, which means it
should have a low performance overhead as the limit is only checked when trying to hit
the global allocator.

To further avoid performance concerns, ChunkFooter has been refactored to keep
track of the total allocated bytes, allowing Bump::allocated_bytes to avoid
performing a walk of the chunk list.

No new new methods were added, and limits must be set after the
creation of a Bump arena. Limits can be changed on the fly and are
only enforced when attempting to allocate new chunks.

No changes were made to the errors returned by operations like alloc
because of backwards compatibility.

I'm new to open source contributions and library development so sorry if there's anything obvious I missed.

fitzgen

Thanks! A few comments below.

src/alloc.rs

src/lib.rs

Eliasin · 2022-06-22T21:11:07Z

Hey @fitzgen, just checking in to see if you've had time to review the latest changes? No worries if you're busy though 😃

fitzgen

Thanks! Somehow this slipped through my email.

I have a few small comments below, but this generally looks good, and after those things are addressed we should be able to merge this.

Thanks for being patient!

src/lib.rs

tests/allocation_limit.rs

Eliasin · 2022-06-24T04:20:16Z

All the comments should be resolved with these commits 😄

Eliasin · 2022-06-24T04:40:14Z

On an unrelated note, between the time I opened the PR and now I started project selection for my 4th year project in school and am looking at doing work exploring the applications of WASM modules for IoT app development and I noticed that you're a maintainer of wasmtime.

I'll hopefully be working with wasmtime and wit-bindgen for this so I might talk to you later on the bytecode alliance zulip 😄.

Eliasin · 2022-06-24T05:22:49Z

All the comments should be resolved with these commits 😄

I just have one concern regarding how allocation limits interact with the value for the minimum chunk size allocation. If a limit is set that is below DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER, all allocations will fail because no sizes will be considered that are below it.

Should set_allocation_limit become fallible to warn about this case? Or should a note be left in the documentation for allocation limits?

Useful documentation notes might have to effectively make the value of DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER public, but because of the details of this implementation, any reductions to DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER could be a breaking change anyway.

fitzgen · 2022-06-27T18:54:02Z

I just have one concern regarding how allocation limits interact with the value for the minimum chunk size allocation. If a limit is set that is below DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER, all allocations will fail because no sizes will be considered that are below it.

Ah this is a good point.

What if we chain on the limit as a size option to new_sizes in this scenario inside alloc_layout_slow:

let sizes = ...;

let sizes = sizes
    .chain(match self.allocation_limit() {
        Some(limit) if limit < DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER
            && self.allocated_bytes() == 0 => Some(limit),
        _ => None,
    });

let new_footer = ...;

And then the existing filter_map and size checking in Bump::new will make this Just Work.

Eliasin · 2022-06-27T23:33:09Z

I just have one concern regarding how allocation limits interact with the value for the minimum chunk size allocation. If a limit is set that is below DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER, all allocations will fail because no sizes will be considered that are below it.

Ah this is a good point.

What if we chain on the limit as a size option to new_sizes in this scenario inside alloc_layout_slow:
let sizes = ...;

let sizes = sizes
    .chain(match self.allocation_limit() {
        Some(limit) if limit < DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER
            && self.allocated_bytes() == 0 => Some(limit),
        _ => None,
    });

let new_footer = ...;
And then the existing filter_map and size checking in Bump::new will make this Just Work.

This mostly works but my testing finds it interacts badly with the page strategy for small allocations and the addition of memory for the footer, breaking the quickcheck for allocation limits never being exceeded.

To make this work I think that either the size check needs to be moved back into new_chunk or the logic for deciding the final layout of the allocated chunk needs to be factored out so it can be used as the size check in alloc_layout_slow.

I think this was technically a problem before the above change and was just masked by how the rounding + overhead + footer size adjustment is not significant for larger limits.

This does have it's own problems though as if the check is just moved around the issue changes from happening when trying to allocate less than DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER to happening when trying to allocate less than some amount which would be rounded up + overhead to over the limit, meaning it might be unclear to the end user why an allocation failed even though there is technically room before hitting the limit.

One way to work around this is to stipulate that the limit can be exceeded by some margin that covers the adjustment that new_chunk makes to requests. From what I can see, the max that new_chunk can decide to allocate over the requested size is 0x1000 - 1 - OVERHEAD when rounding large allocations to a page boundary, this margin of error may be unacceptably large though.

Eliasin · 2022-06-29T17:05:53Z

@fitzgen Since I'm not sure if plain comments trigger notifications

Eliasin · 2022-06-30T23:17:20Z

I took a stab at fixing this problem by factoring out the logic for determining the final size of chunk allocations so that the code in alloc_layout_slow could determine the exact size of a new allocated chunk. This fixes breaking the limit because of the difference in allocation request size and the final size.

I also allowed the chunk size halving process to go below the min chunk size when dealing with small limits as just requesting a chunk with the limit value usually rounds up past the limit and ends up being rejected.

Eliasin · 2022-06-30T23:18:03Z

I'm realizing now that this change causes some memory details about the new chunks to be calculated twice in a lot of cases so I'll make a change to avoid that now.

Eliasin · 2022-06-30T23:38:57Z

I'm realizing now that this change causes some memory details about the new chunks to be calculated twice in a lot of cases so I'll make a change to avoid that now.

I've made the change to cache the new chunk memory details so that they are not calculated twice but this necessitated a change in new_chunk, marking it as unsafe because of it no longer can verify (without recalculating and defeating the point) the safety of the new chunk memory details passed to it.

I'm not sure if that's an acceptable change and I'm not a fan of the name I have NewChunkMemoryDetails but I wanted to get a solution out and see what your opinion is.

Eliasin · 2022-06-30T23:48:55Z

src/lib.rs

+            let chunk_memory_details = iter::from_fn(|| {
+                let bypass_min_chunk_size_for_small_limits = match self.allocation_limit() {
+                    Some(limit)
+                        if layout.size() < limit
+                            && base_size >= layout.size()
+                            && limit < DEFAULT_CHUNK_SIZE_WITHOUT_FOOTER
+                            && self.allocated_bytes() == 0 =>
+                    {
+                        true
+                    }
+                    _ => false,
+                };
+
+                if base_size >= min_new_chunk_size || bypass_min_chunk_size_for_small_limits {
                    let size = base_size;
                    base_size = base_size / 2;
-                    Some(size)
+                    Bump::new_chunk_memory_details(Some(size), layout)


I'm not sure if rustc is smart enough to transform this into

if base_size >= min_new_chunk_size { // ... } else { let bypass_min_chunk_size_for_small_limits = ...; if bypass_min_chunk_size_for_small_limits { let size = base_size; base_size = base_size / 2; Bump::new_chunk_memory_details(Some(size), layout) } else { None } }

but I decided to hold off on any premature optimizations since I think this is much less readable

fitzgen

Thanks! This looks good modulo a couple small things below.

src/lib.rs

tests/allocation_limit.rs

fitzgen

Thanks! And thanks for your patience getting this one 100% over the finish line!

fitzgen · 2022-07-07T18:14:00Z

https://github.com/fitzgen/bumpalo/runs/7239112120?check_suite_focus=true#step:5:126

Looks like abs_diff wasn't introduced until Rust 1.60, and our MSRV is 1.50. Mind open coding abs_diff here?

Eliasin · 2022-07-07T23:43:52Z

@fitzgen Should be good now, I ran the tests on 1.54.0

fitzgen · 2022-07-11T16:09:21Z

Looks like CI is still failing:

https://github.com/fitzgen/bumpalo/runs/7286188380?check_suite_focus=true#step:5:342

failures:
---- oom_instead_of_bump_pointer_overflow stdout ----
Layout::from_size_align errored: invalid parameters to Layout::from_size_align
note: test did not panic as expected
error: test failed, to rerun pass '--test tests'
failures:
    oom_instead_of_bump_pointer_overflow

Eliasin · 2022-07-11T18:31:08Z

Looks like CI is still failing:

https://github.com/fitzgen/bumpalo/runs/7286188380?check_suite_focus=true#step:5:342
failures:
---- oom_instead_of_bump_pointer_overflow stdout ----
Layout::from_size_align errored: invalid parameters to Layout::from_size_align
note: test did not panic as expected
error: test failed, to rerun pass '--test tests'
failures:
    oom_instead_of_bump_pointer_overflow

@fitzgen I think this might be a nightly breakage as I ran this test on upstream main with nightly and experienced a failure

Eliasin · 2022-07-11T18:39:00Z

Looks like CI is still failing:
https://github.com/fitzgen/bumpalo/runs/7286188380?check_suite_focus=true#step:5:342
failures:
---- oom_instead_of_bump_pointer_overflow stdout ----
Layout::from_size_align errored: invalid parameters to Layout::from_size_align
note: test did not panic as expected
error: test failed, to rerun pass '--test tests'
failures:
    oom_instead_of_bump_pointer_overflow
@fitzgen I think this might be a nightly breakage as I ran this test on upstream main with nightly and experienced a failure

The test passes on this PR and upstream main on the nightly of July 10th 2022 but not July 11th 2022, which is today

Eliasin · 2022-07-11T19:10:46Z

Toying with the latest nightly it seems that Layout::size_from_align panics with a lot of values that stable won't with. Namely values of size near usize::MAX. I'm not sure if this constitutes a nightly regression or where to even report something like this though

Eliasin · 2022-07-11T19:28:35Z

I asked on the rust zulip and it seems like there is a new requirement for Layout::from_size_align that the size value must fit in an isize from this PR which the nightly docs now show so the test will have to be changed

Eliasin · 2022-07-11T19:45:08Z

I made an issue for the breakage with a tentative suggested change

Eliasin · 2022-07-26T23:38:09Z

Hey @fitzgen , just pinging in case this slipped by your email 😃

fitzgen · 2022-07-27T21:27:51Z

I don't have time to look into the old tests at the moment, if you can get them passing and send a PR then I can review that.

fitzgen · 2022-08-04T17:46:06Z

I think this should pass CI now if rebased

Arenas can optionally keep track of an `allocation_limit` which is considered when allocating new chunks to an arena. No new `new` methods were added, and limits must be set after the creation of a `Bump` arena. Limits can be changed on the fly and are only enforced when attempting to allocate new chunks. No changes were made to the errors returned by operations like `alloc` because of backwards compatibility.

Clarify that allocation limits are per `Bump` arena Co-authored-by: Nick Fitzgerald <[email protected]>

ChunkFooter now tracks the allocated bytes, reducing the operation of retrieving the allocated bytes to a field lookup instead of a linear walk of the chunk list.

Co-authored-by: Nick Fitzgerald <[email protected]>

To ensure that allocations do not exceed the allocation limit imposed on a `Bump`, the logic for calculating the final size of an allocated chunk was factored out into a separate function so that the limit enforcing code higher up in the control flow could more strictly enforce the allocation limit, as previously this code did not have visibility into the final size of an allocation request. To ensure that allocations in arenas with small limits could succeed, `alloc_layout_slow` was changed to allow bypassing the minimum chunk size for new chunks when trying to allocate in new arenas when the limit was lower than the default minimum chunk size.

When allocating new chunks, the memory layout for new chunks was calculated twice, once for the code checking if the new chunk would violate allocation limits and once for the actual code allocating the new chunk. This change caches the calculated new chunk memory layout inbetween these steps to avoid the extra calculation. This change means that `new_chunk` must be marked `unsafe` as before this change `new_chunk` could guarantee that the memory details of a new chunk were safe (correct alignment, size, etc.), now that it is not responsible for calculating these details, callers can cause unsoundness through misuse of `new_chunk`.

Eliasin · 2022-08-05T04:45:10Z

I think this should pass CI now if rebased

Seems like it does!

fitzgen · 2022-08-05T17:08:08Z

Thanks! And thanks for your patience!

Eliasin · 2022-08-05T20:40:51Z

I had a great time working on this PR, thanks for your work reviewing it and maintaining this crate! 😄

fitzgen reviewed Jun 6, 2022

View reviewed changes

Eliasin force-pushed the main branch 3 times, most recently from ce26866 to 4e3a178 Compare June 9, 2022 01:10

Eliasin requested a review from fitzgen June 9, 2022 01:13

fitzgen reviewed Jun 23, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

tests/allocation_limit.rs Outdated Show resolved Hide resolved

tests/allocation_limit.rs Outdated Show resolved Hide resolved

Eliasin force-pushed the main branch 4 times, most recently from f3fb2b0 to 8efa890 Compare June 24, 2022 04:14

Eliasin requested a review from fitzgen June 24, 2022 04:20

Eliasin force-pushed the main branch from 921b961 to 7f47641 Compare June 30, 2022 23:10

Eliasin commented Jun 30, 2022

View reviewed changes

fitzgen reviewed Jul 5, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

tests/allocation_limit.rs Outdated Show resolved Hide resolved

Eliasin requested a review from fitzgen July 6, 2022 00:48

fitzgen approved these changes Jul 7, 2022

View reviewed changes

Eliasin and others added 17 commits August 4, 2022 17:09

Undo change to AllocError documentation

22b8973

Make wording around allocation limits more specific

5f0bf99

Clarify that allocation limits are per `Bump` arena Co-authored-by: Nick Fitzgerald <[email protected]>

Simplify API for changing allocation limit on Bump

0046056

Remove unnecessary inline hints

4456aab

Refactor allocated_bytes to avoid walking chunk list

33d1488

ChunkFooter now tracks the allocated bytes, reducing the operation of retrieving the allocated bytes to a field lookup instead of a linear walk of the chunk list.

Clarify allocation limit enforcement

72e3573

Co-authored-by: Nick Fitzgerald <[email protected]>

Fix bump allocation limit section documentation at wrong level

bdaad5c

Co-authored-by: Nick Fitzgerald <[email protected]>

Rename allocation_limit quickcheck test parameter for better clarity

f13bd35

Refactor where new chunks are checked against limits

29e9c2a

Remove unnecessary flatten

00256c2

Replace unwrap_or with and_then in allocation limit calculation

00672c6

Remove stale comment

cc79669

Add comment for NewChunkMemoryDetails

535da6c

Change use of usize::abs_diff due to MSRV

2339102

Eliasin force-pushed the main branch from 3dbefac to 2339102 Compare August 4, 2022 21:11

fitzgen merged commit f551c37 into fitzgen:main Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for allocation limits on arenas #160

Add support for allocation limits on arenas #160

Eliasin commented Jun 2, 2022 •

edited

Loading

fitzgen left a comment

Eliasin commented Jun 22, 2022 •

edited

Loading

fitzgen left a comment

Eliasin commented Jun 24, 2022

Eliasin commented Jun 24, 2022

Eliasin commented Jun 24, 2022

fitzgen commented Jun 27, 2022

Eliasin commented Jun 27, 2022 •

edited

Loading

Eliasin commented Jun 29, 2022

Eliasin commented Jun 30, 2022

Eliasin commented Jun 30, 2022

Eliasin commented Jun 30, 2022

Eliasin Jun 30, 2022

fitzgen left a comment

fitzgen left a comment

fitzgen commented Jul 7, 2022

Eliasin commented Jul 7, 2022

fitzgen commented Jul 11, 2022

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022

Eliasin commented Jul 26, 2022 •

edited

Loading

fitzgen commented Jul 27, 2022

fitzgen commented Aug 4, 2022

Eliasin commented Aug 5, 2022

fitzgen commented Aug 5, 2022

Eliasin commented Aug 5, 2022

Add support for allocation limits on arenas #160

Add support for allocation limits on arenas #160

Conversation

Eliasin commented Jun 2, 2022 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

Eliasin commented Jun 22, 2022 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

Eliasin commented Jun 24, 2022

Eliasin commented Jun 24, 2022

Eliasin commented Jun 24, 2022

fitzgen commented Jun 27, 2022

Eliasin commented Jun 27, 2022 • edited Loading

Eliasin commented Jun 29, 2022

Eliasin commented Jun 30, 2022

Eliasin commented Jun 30, 2022

Eliasin commented Jun 30, 2022

Eliasin Jun 30, 2022

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Jul 7, 2022

Eliasin commented Jul 7, 2022

fitzgen commented Jul 11, 2022

Eliasin commented Jul 11, 2022 • edited Loading

Eliasin commented Jul 11, 2022 • edited Loading

Eliasin commented Jul 11, 2022

Eliasin commented Jul 11, 2022 • edited Loading

Eliasin commented Jul 11, 2022

Eliasin commented Jul 26, 2022 • edited Loading

fitzgen commented Jul 27, 2022

fitzgen commented Aug 4, 2022

Eliasin commented Aug 5, 2022

fitzgen commented Aug 5, 2022

Eliasin commented Aug 5, 2022

Eliasin commented Jun 2, 2022 •

edited

Loading

Eliasin commented Jun 22, 2022 •

edited

Loading

Eliasin commented Jun 27, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 11, 2022 •

edited

Loading

Eliasin commented Jul 26, 2022 •

edited

Loading