MAINT: Simplify string arena growth strategy #29885
Conversation
| if ((arena->cursor + string_storage_size) > newsize) { | ||
| // need extra room beyond the expansion factor, leave some padding | ||
| newsize = (size_t)(ARENA_EXPAND_FACTOR * (arena->cursor + string_storage_size)); | ||
| if ((arena->size - arena->cursor) < string_storage_size) { |
There was a problem hiding this comment.
I changed the <= to < here. == seems fine, i.e. at the end size == cursor which should be valid, no?
|
Thanks for looking at this! Just an FYI I am traveling in Portugal this week and may not have time to look at this until next week. |
Not sure if there is any downsides to this, but I think this is much simpler and pretty much identical. Plus, I don't quite like that the 1.25 factor could be large, although I admit it seems very unlikely (maybe impossible) to actually trigger a problem there. Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
seberg
force-pushed
the
string-overalloc
branch
from
a782ef7 to
b36990a
Compare
|
Thanks for looking at this. I agree that the original code has issues and your fix is fine. In your edit:
This seems like a good idea too and I like the idea of having a utility function we can use for growing buffers. If you're up for it, it would be nice to re-use that code here. But also this is fine to merge as-is IMO. |
|
I'll just put this in as back-portable, although dunno if we will. May look into consolidating, but that would make the change unnecessarily bloated for backporting (if it turns out nice). |
Not sure if there is any downsides to this, but I think this is much simpler and pretty much identical. Plus, I don't quite like that the 1.25 factor could be large, although I admit it seems very unlikely (maybe impossible) to actually trigger a problem there. Signed-off-by: Sebastian Berg <sebastianb@nvidia.com>
MAINT: Simplify string arena growth strategy (#29885)
2 participants
Not sure if there is any downsides to this, but I think this is much simpler and pretty much identical.
Plus, I don't quite like that the 1.25 factor could be large, although I admit it seems very unlikely (maybe impossible) to actually trigger a problem there.
EDIT: As a note, I have a similar function that caps overallocation and increments in "neat" values (which double as a minimum overallocation amount). I somewhat assumed that doesn't matter much, but we could definitely do either or both here as well.