alt Hacker NewsEdit: I'm getting a lot of down votes for this but nobody is saying why I'm wrong. If you think I'm wrong enough to down vote, please reply why.
I don't understand why that is the preferred fix. I would have solved it other ways:
1. When resizing the page, leave some flag of how it was allocated. This tagging is commonly done as the always 0 bits in size or address fields to save space.
2. Since the pool is a known size of contiguous memory, check if the memory to be freed is within that range
3. Make the size immutable. If you want to realloc, go for it, and have the memory manager handle that boundary for you.
Both of those not only maintain functionality which seems to have been lost with the feature reduction but also are more future proof to any other changes in size.
Replies
I upvoted you because I would like to know the response to these approaches
I didn't downvote, but I suspect it's an easy answer: the fix was like four lines.
At the end of the day, #1 and #3 both probably add a fairly significant amount of code and complexity that it's not clear to me adds robustness or clarity. From the fix:
``` // If our first node has non-standard memory size, we can't reuse // it. This is because our initBuf below would change the underlying // memory length which would break our memory free outside the pool. // It is easiest in this case to prune the node. ```
https://github.com/ghostty-org/ghostty/commit/17da13840dc71b...
#3, it seems, would require making a broader change. The size effectively is immutable now (assuming I'm understanding your comment correctly): non-standard pages never change size, they get discarded without trying to change their size.
#2 is interesting, but I think it won't work because the implementation of MemoryPool doesn't seem like it would make it easy to test ownership:
https://github.com/ghostty-org/ghostty/blob/17da13840dc71ba3...
You'd have to make some changes to be able to check the arena buffers, and that check would be far slower than the simple comparison.