Fix MBR allocations for >= 1024 bytes #383
Conversation
When trying to allocate a single object with size >= 1024, the current implementation crashes with a stack overflow. This patch allows blocks to exceed the hard-coded size limit to accommodate larger objects.
| return res; | ||
| } else { | ||
| allocate_block(current_); | ||
| return allocate(num_bytes, alignment); |
There was a problem hiding this comment.
Is this where the recursion happened? Nothing to do, just curious.
There was a problem hiding this comment.
Yep, that's where it blew up.
| @@ -11,7 +11,7 @@ namespace broker::detail { | |||
| class monotonic_buffer_resource { | |||
| public: | |||
| monotonic_buffer_resource() { | |||
| allocate_block(nullptr); | |||
| allocate_block(nullptr, 0); | |||
There was a problem hiding this comment.
Could this be dropped?
Not new, but leaving a comment anyhow: Might be surprising to allocate memory by default even if the mbr doesn't end-up being used and this may throw bad_alloc() surprisingly, too.
Also, if the first object allocated is more than block_size, this wastes one block of memory.
The std one doesn't behave this way it seems:
https://en.cppreference.com/w/cpp/memory/monotonic_buffer_resource/monotonic_buffer_resource
There was a problem hiding this comment.
Also, if the first object allocated is more than
block_size, this wastes one block of memory.
True, but for the use cases we have, Broker will always have something "small" to write first. In the variant branch also (were it blew up).
We can't just drop this first allocation, because allocate assumes that current_ is always valid. So I'd have at least to add some if-guard there (and maybe in other places). Want me to change it?
There was a problem hiding this comment.
We can't just drop this first allocation, because allocate assumes that current_ is always valid. So I'd have at least to add some if-guard there (and maybe in other places). Want me to change it?
No worries, no need to change anything. Thought if _remaining = 0 then std::align would do the "right" thing, but didn't look further.
There was a problem hiding this comment.
All right, merged. Thanks for the fast review! 🙏
When trying to allocate a single object with size >= 1024, the current implementation crashes with a stack overflow. This patch allows blocks to exceed the hard-coded size limit to accommodate larger objects.
I'll also check whether we can get rid of this class entirely as a followup. It's just a placeholder until we can use std::pmr::monotonic_buffer_resource.