3 ms·

Edit: 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 unders

by Neywiny 10mo ago

Edit: 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.

hotpotat 10mo ago
I upvoted you because I would like to know the response to these approaches
- Neywiny 10mo ago
  Thank you. Sometimes I get to like -4 or even -7 before it starts going up. It might be nice to graph it at some point to see my most varied comments. I'm at -2 right now 23 minutes later I'm at +2 6 minutes after, +5 +4min now +6, another 20 minutes +8. I think I'm in the clear
  - yakaccount4 10mo ago
    I just stopped caring about votes. It's often driven by inertia, and it can't differentiate a vote from someone who doesn't know anything vs a domain expert. Life is better once you stop caring about karma points.
    - Neywiny 10mo ago
      While very true and sound advice, fake internet points make dopamine go brrrrr
  - deleted 10mo ago
    [deleted]
bastawhiz 10mo ago
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/17da13840dc71ba36b0deb2c0e85097840630ad5#diff-f8240ae43b0be54aa9eeb03a2d7139d08a4b18747402eec8c43535b6ce220eb7R2541 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/17da13840dc71ba36b0deb2c0e85097840630ad5/src/terminal/PageList.zig 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.
- Neywiny 10mo ago
  Thank you. I think each of my options are pretty trivial in C. I guess what I'm not understanding for #3 is if size is immutable, how the size changed which caused the issue? The post said they changed the size of the page without changing the underlying size of the allocated memory. To me this is the big issue. There was a desync in information where the underlying assumption is that size tells you where the data came from and that the size of the metadata and the size of the allocation move in tandem across that boundary. #1 and #2 are fixes for breaking that implicit trust. #1 still trusts the metadata, #2 is what I'd consider the most robust solution is that not only is it ideally trivial (just compare if a pointer is within a range, assuming zig can do that) but it doesn't rely on metadata being correct. #3 prevents the desync. I really don't understand the code base enough to say definitively that my ways work, which is I guess what I'm really looking for feedback on. Looking at the memorypool, I think you're right that my assumption of it being a simple contiguous array was incorrect. ETA: I think I'm actually very wrong for #2. Color me surprised that the zig memory pool allocated each item separately instead of as one big block. Feels like a waste, but I'm sure they have their reasons. That's addCapacity in memory_pool.zig
deleted 10mo ago
[deleted]

by Neywiny 10mo ago

hotpotat 10mo ago

I upvoted you because I would like to know the response to these approaches

Neywiny 10mo ago
Thank you. Sometimes I get to like -4 or even -7 before it starts going up. It might be nice to graph it at some point to see my most varied comments. I'm at -2 right now 23 minutes later I'm at +2 6 minutes after, +5 +4min now +6, another 20 minutes +8. I think I'm in the clear
- yakaccount4 10mo ago
  I just stopped caring about votes. It's often driven by inertia, and it can't differentiate a vote from someone who doesn't know anything vs a domain expert. Life is better once you stop caring about karma points.
  - Neywiny 10mo ago
    While very true and sound advice, fake internet points make dopamine go brrrrr
- deleted 10mo ago
  [deleted]

bastawhiz 10mo ago

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/17da13840dc71ba36b0deb2c0e85097840630ad5#diff-f8240ae43b0be54aa9eeb03a2d7139d08a4b18747402eec8c43535b6ce220eb7R2541 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/17da13840dc71ba36b0deb2c0e85097840630ad5/src/terminal/PageList.zig 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.

Neywiny 10mo ago
Thank you. I think each of my options are pretty trivial in C. I guess what I'm not understanding for #3 is if size is immutable, how the size changed which caused the issue? The post said they changed the size of the page without changing the underlying size of the allocated memory. To me this is the big issue. There was a desync in information where the underlying assumption is that size tells you where the data came from and that the size of the metadata and the size of the allocation move in tandem across that boundary. #1 and #2 are fixes for breaking that implicit trust. #1 still trusts the metadata, #2 is what I'd consider the most robust solution is that not only is it ideally trivial (just compare if a pointer is within a range, assuming zig can do that) but it doesn't rely on metadata being correct. #3 prevents the desync. I really don't understand the code base enough to say definitively that my ways work, which is I guess what I'm really looking for feedback on. Looking at the memorypool, I think you're right that my assumption of it being a simple contiguous array was incorrect. ETA: I think I'm actually very wrong for #2. Color me surprised that the zig memory pool allocated each item separately instead of as one big block. Feels like a waste, but I'm sure they have their reasons. That's addCapacity in memory_pool.zig

deleted 10mo ago

[deleted]