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

61–70 of 152 posts

Re: Finding and fixing Ghostty's largest memory leak

#61

Post by Neywiny »

bastawhiz wrote: post #50
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…

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

Re: Finding and fixing Ghostty's largest memory leak

#62

Post by aw1621107 »

tialaramex wrote: post #58
woodruffw wrote: post #36

Earlier quoted context omitted.

You’ll probably be waiting a long time, since Rust very explicitly doesn’t have “leak safety” as a constructive property. Safe Rust programs are allowed to leak memory, because memory leaks themselves don’t cause safety issues. There’s even a standard, non-unsafe API for leaking memory[1]. (What Rust does do is make it harder to construct programs that leak memory unintentionally. It’s possible but not guaranteed tha…

The specific language feature you want if you insist that you don't want this kind of leak is Linear Types. Rust has Affine Types. This means Rust cares that for any value V of type T, Rust can see that we did not destroy V twice (or more often). With Linear Types the compiler checks that you destroyed V exactly once, not less and not more. However, one reason I don't end up caring about Leak Safety of this sort is t…

> I think the main part of Ghostty's design mentioned here that - as a Rust programmer - I think is probably a mistake is the choice to use a linked list. To me this looks exactly like it needs VecDeque, a circular buffer backed by a growable array type.

This comment [0] by mitchellh on the corresponding lobste.rs submission discusses the choice of data structure a bit more:

> Circular buffer is a pretty standard approach to this problem. I think it's what most terminal emulators do.

> The reason I went with this doubly linked list approach with Ghostty is because architecturally it makes it easier for us to support some other features that either exist or are planned.

> As an example of planned, one of the most upvoted feature requests is the ability for Ghostty to persist scroll back across relaunch (macOS built-in terminal does this and maybe iTerm2). By using a paged linked list architecture, we can take pages that no longer contain the active area (and therefore are read-only) and archive them off the IO thread during destroy when we need to prune scroll back. We don't need to ever worry that the IO thread might circle around and produce a read/write data race.

> Or another example that we don't do yet, we can convert the format of scroll back history into a much more compressed form (maybe literally compressed memory using something like zstd) so we can trade off memory for cpu if users are willing to pay a [small, probably imperceptible] CPU time cost when you scroll up.

[0]: https://lobste.rs/s/vlzg2m/finding_fixing_ghostty_s_largest_...

Re: Finding and fixing Ghostty's largest memory leak

#63

Post by vlovich123 »

tialaramex wrote: post #58
woodruffw wrote: post #36

Earlier quoted context omitted.

You’ll probably be waiting a long time, since Rust very explicitly doesn’t have “leak safety” as a constructive property. Safe Rust programs are allowed to leak memory, because memory leaks themselves don’t cause safety issues. There’s even a standard, non-unsafe API for leaking memory[1]. (What Rust does do is make it harder to construct programs that leak memory unintentionally. It’s possible but not guaranteed tha…

The specific language feature you want if you insist that you don't want this kind of leak is Linear Types. Rust has Affine Types. This means Rust cares that for any value V of type T, Rust can see that we did not destroy V twice (or more often). With Linear Types the compiler checks that you destroyed V exactly once, not less and not more. However, one reason I don't end up caring about Leak Safety of this sort is t…

The issue isn’t linked list vs dequeue but type confusion about what was in the container. They didn’t forget to drop it - they got confused about which type was in the list when popping and returned it to the pool instead of munmap.

The way to solve this in Rust would be to put this logic in the drop and hide each page type in an enum. That way you can’t ever confuse the types or what happens when you drop.

Re: Finding and fixing Ghostty's largest memory leak

#64

Post by trevorhinesley »

stephc_int13 wrote: post #48

I've been following the development of Ghostty for a while and while I have the feeling that there is a bit of over-engineering in this project, I find this kind of bug post mortem to be extremely valuable for anyone in love with the craft.

Over-engineered in what way?

Re: Finding and fixing Ghostty's largest memory leak

#65

Post by bastawhiz »

Neywiny wrote: post #61
bastawhiz wrote: post #50

Earlier quoted context omitted.

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…

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

I'm not 100%, but my understanding was that the non standard pages are always larger than the standard pages. If you need more than a standard page, you always get a freshly allocated non standard page. But when one was released, it was being treated as though it was standard sized. The pool would then reuse that memory, but only at a standard size. So every released non standard page leaked the difference between what was allocated and what was standard.

Which is to say, I don't think it was actually being resized. I think it was the metadata for the page saying it had the (incorrect) standard size (and the incorrect handling after the metadata was changed).

Re: Finding and fixing Ghostty's largest memory leak

#67

Post by DrammBA »

msephton wrote: post #55
jrpelkonen wrote: post #25

Great write-up. And, thanks mitchellh for Ghostty, I switched to it last year, and have not regretted it. However, I am a somewhat surprised that the fix is reserved for a feature release in a couple of months. I would have expected this to be included in a bug fix release.

It's already released in the latest nightly build.

Are the nightly releases the expected way to get timely bugfixes?

Re: Finding and fixing Ghostty's largest memory leak

#68

Post by [deleted] »

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…

[deleted]

Re: Finding and fixing Ghostty's largest memory leak

#70

Post by autarch »

sean_pedersen wrote: post #69

Would this kind of bug have been catched by the Rust compiler?

I was wondering about this myself. My guess is no, since AFAIK the only way to do this sort manual memory management is to use unsafe code. But there's also things like the (bumpalo)[https://docs.rs/bumpalo/latest/bumpalo] crate in Rust, so maybe you wouldn't need to do this sort of thing by hand, in which case you're as leak-free as the bumpalo crate.
Post reply on HN