fitzgen / bumpalo

A fast bump allocation arena for Rust
https://docs.rs/bumpalo
Apache License 2.0
1.42k stars 112 forks source link

[WIP] Added try_alloc_layout, try_reserve, & try_reserve_exact #67

Closed TheDan64 closed 4 years ago

TheDan64 commented 4 years ago

1) Made it a point to not make any breaking changes to the public API. 2) I implemented my own global allocator which always returns a null ptr to simulate OOM and it seemed like Bump::new() / Bump::with_capacity(0) would always panic with oom(). Shouldn't it not try and reach for the allocator in that case? Any idea how to go about fixing that special case? 3) I named the new methods try_reserve and try_reserve_exact to match what raw/vec calls them, but I could rename them to try_alloc_in_new_chunk_if_necessary like you suggested if you wish.

Fixes #66

fitzgen commented 4 years ago

I implemented my own global allocator which always returns a null ptr to simulate OOM and it seemed like Bump::new() / Bump::with_capacity(0) would always panic with oom(). Shouldn't it not try and reach for the allocator in that case? Any idea how to go about fixing that special case?

For these cases, we should add try_with_capacity and try_new variants that return Result<Self>. Bump isn't designed to exist without a chunk to allocate into, which would require new branches in the allocation fast path.

fitzgen commented 4 years ago

Please leave a comment here when this is ready for review again -- thanks!

TheDan64 commented 4 years ago

I added the test you roughed out, but it seems to make Rust go OOM in the middle of the loop, not sure why Rust internals are trying to allocate there.

Edit: I think the asserts are allocating strings for error messages

TheDan64 commented 4 years ago

Did you have a chance to poke around at the test?

fitzgen commented 4 years ago

Did you have a chance to poke around at the test?

Unfortunately I don't have time to look at this PR this week, and probably won't until wednesday or thursday next week.

In the meantime, I was thinking it may make sense to have a specialized assert! macro for inside this test file that automatically turns the global allocator on again if the assertion condition triggers, but before the panic actually happens.

TheDan64 commented 4 years ago

I think a problem that I'm running into is that cargo will run tests in parallel, so multiple try_alloc.rs tests will interfere with each other. And disabling parallel tests for the entire test suite seems less than ideal. So maybe something like a mutex or any other lock really is ideal so that each test can be guaranteed exclusive access to the allocator.

TheDan64 commented 4 years ago

And I'm also not convinced that the separate file separates the allocator from the other existing tests. I did see one tests randomly go OOM: thread 'alloc_slice_overflow' panicked at 'out of memory', src/lib.rs:1104:5

Edit: Nevermind, that test is supposed to fail. Maybe I was just seeing the nocapture output of it

TheDan64 commented 4 years ago

Hey, @fitzgen, any chance you'll have an opportunity to look at this? Would like to see this to completion :)

fitzgen commented 4 years ago

I don't have time to investigate further myself right now -- if you hammer out the kinks, I'm happy to review tho!

TheDan64 commented 4 years ago

@fitzgen I think this is ready for final review now. Unfortunately I had to combine my two tests into one, as tests are run in parallel by default and so the global allocator would toggle during one test, potentially affecting the other. Using a Mutex in the allocator doesn't help because it needs to be locked for the entirety of a single test, and Rust accesses it "async"ly. Hypothetically this could be accomplished with a thread-local allocator, but this isn't supported by Rust (not sure if that's a thing in general).

fitzgen commented 4 years ago

Thanks!

fitzgen commented 4 years ago

FWIW, I split the tests up again by writing a custom main function, rather than using the default test harness. This is usually overkill, but I found that the tests were super fragile without it (as we also saw here with the difficulty of getting it green in the first place).

fitzgen commented 4 years ago

Also, I published 3.3.0 with these changes.