Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Finding and fixing Ghostty's largest memory leak

mitchellh.com

Post reply on HN
Print view View original

41–50 of 152 posts

Re: Finding and fixing Ghostty's largest memory leak

#41

Post by fragmede »

mitchellh wrote: post #6
quantummagic wrote: post #2

This is great news! Well done to everyone who helped sort it out. It was a problem noted by users in a thread here just last week, https://news.ycombinator.com/item?id=46460319 While Claude Code might have been the reason this bug became triggered by more people, there are some of us who were hitting it without ever having used Claude Code at all. Maybe the assumption about what makes a page non-standard, isn't as bl…

> Probably not a huge deal, but it does seem the fix will needlessly delete and recreate non-standard pages in the case where the new page needs to be non-standard, and the oldest one (that needs to be pruned) already is non-standard and could be reused. This is addressed in the blog post. It is how the PageList has always worked, and also how it worked before with the bug, because during capacity adjustment we would…

Of all the things to be impressed by you about, your patience is commendable. I'd be losing my shit if someone couldn't be bothered to read what I wrote and just spout off about something I'd addressed in my writing, but I suppose that's why your bank account has two commas and a bunch more. Thank you for everything. Can we go flying sometime?

Re: Finding and fixing Ghostty's largest memory leak

#42

Post by Neywiny »

hotpotat wrote: post #37
Neywiny wrote: post #31

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 s…

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

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

Re: Finding and fixing Ghostty's largest memory leak

#43

Post by flumpcakes »

cyberax wrote: post #38

Ugh. Is it just me, or is anyone else feeling a tad uncomfortable that their terminal app needs a custom memory allocator that mucks with low-level page tags?

I am not sure on what your commented is based on, but in short: No? High performance software needs to deal with memory, and optimisations often will need some kind of direct control - as in this example where re-using memory is more performant than constantly churning with mmap.

Re: Finding and fixing Ghostty's largest memory leak

#45

Post by yakaccount4 »

Neywiny wrote: post #42
hotpotat wrote: post #37

Earlier quoted context omitted.

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

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

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.

Re: Finding and fixing Ghostty's largest memory leak

#46

Post by Neywiny »

yakaccount4 wrote: post #45
Neywiny wrote: post #42

Earlier quoted context omitted.

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

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.

While very true and sound advice, fake internet points make dopamine go brrrrr

Re: Finding and fixing Ghostty's largest memory leak

#47

Post by [deleted] »

Neywiny wrote: post #42
hotpotat wrote: post #37

Earlier quoted context omitted.

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

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

[deleted]

Re: Finding and fixing Ghostty's largest memory leak

#49

Post by sequin »

flumpcakes wrote: post #43
cyberax wrote: post #38

Ugh. Is it just me, or is anyone else feeling a tad uncomfortable that their terminal app needs a custom memory allocator that mucks with low-level page tags?

I am not sure on what your commented is based on, but in short: No? High performance software needs to deal with memory, and optimisations often will need some kind of direct control - as in this example where re-using memory is more performant than constantly churning with mmap.

I honestly don't understand why a terminal emulator needs to be performant. Seems like peak bikeshedding to me.

Re: Finding and fixing Ghostty's largest memory leak

#50

Post by bastawhiz »

Neywiny wrote: post #31

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 s…

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.

Post reply on HN