Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unique: Handle memory leaks; cleanup goroutine can't keep up #71772

Open
bradfitz opened this issue Feb 16, 2025 · 23 comments
Open

unique: Handle memory leaks; cleanup goroutine can't keep up #71772

bradfitz opened this issue Feb 16, 2025 · 23 comments
Assignees
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Feb 16, 2025

Go version

go version go1.24.0 darwin/arm64
go version go1.24.0 linux/amd64
go version go1.23.6 darwin/arm64
go version go1.23.6 linux/amd64

What did you do?

We have a server that processes a stream of user connect & disconnect events from our edge servers. Users are identified by a [32]byte public key, wrapped up like:

type NodePublic struct {
   k [32]byte
}

This server maintains ~4 maps keyed by NodePublic. Out of curiosity and as an experiment, we decided to kick the tires with unique.Handle to make those maps instead be keyed by unique.Handle[NodePublic] to make them 8 bytes instead of 32. It saves about 96 bytes per connection. Not a ton, but ... an experiment.

The experiment didn't go as expected.

What did you see happen?

What we saw happen was unbounded memory growth and kernel OOMs. These are the two instances of this server we run. (it's an internal debug tool only, so we only run two for HA reasons)

Image

It previously took about ~1GB of memory at steady state. With unique.Handle, it just keeps growing until the kernel OOM kills it.

The flat parts in the graph are where we changed the map key types from unique.Handle[NodePublic] back to just NodePublic, changing nothing else.

To debug, I hacked up the unique package (patch: https://gist.github.com/bradfitz/06f57b1515d58fb4d8b9b4eb5a84262e) to try out two things:

  • see if the weak pointers were getting tinyalloc'ed and preventing GC. To counter this, I tried making the unique package use 24 byte value types in its uniqueMap. No change.
  • to log stats when the cleaners ran to see how much it tried to delete
  • Oh, and I also tried both Go 1.23.6 and Go 1.24.0

The result is that never frees anything. The weak pointer is never nil. It takes ever longer to iterate over the map (up to 2.7 seconds now), never finding anything to delete:

Feb 16 00:30:44 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  16µs
Feb 16 00:30:46 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2829766 ; deleted= 0 ; kept= 2829766 ; took  2.585244s
Feb 16 00:32:51 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  14µs
Feb 16 00:32:53 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2850211 ; deleted= 0 ; kept= 2850211 ; took  2.692595s
Feb 16 00:34:58 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  19µs
Feb 16 00:35:01 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2870410 ; deleted= 0 ; kept= 2870410 ; took  2.728681s
Feb 16 00:37:06 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  19µs
Feb 16 00:37:08 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2890817 ; deleted= 0 ; kept= 2890817 ; took  2.635071s
Feb 16 00:39:13 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  18µs
Feb 16 00:39:16 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2911098 ; deleted= 0 ; kept= 2911098 ; took  2.600478s
Feb 16 00:41:20 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  19µs
Feb 16 00:41:22 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2931399 ; deleted= 0 ; kept= 2931399 ; took  2.606853s
Feb 16 00:43:27 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2 ; deleted= 0 ; kept= 2 ; took  20µs
Feb 16 00:43:29 derptrackd2 derptrackd[771925]: XXX did unique cleanup loop: items= 2951574 ; deleted= 0 ; kept= 2951574 ; took  2.701304s

Looking at historical logs, it never deletes anything on Linux.

I tried to write a repro (on my Mac) and I failed, despite a bunch of efforts.

I ended up doing too much debugging on a remote AWS VM, with a hacked up Go toolchain.

Eventually I ran the same code on my Mac (same server + same hacked up Go toolchain), pulling from the same data sources, and surprisingly: it worked! unique finds weak things to delete in its cleaner and it doesn't grow.

In summary:

  • unique leaks on linux/amd64 (prod cloud VM)
  • same code on same data doesn't leak on darwin/arm64 (my dev laptop)

I'll now try revisiting my standalone repro but running it on Linux instead of macOS.

I had hoped to deliver @mknyszek a bug on a silver platter before filing, but I'm going a little crazy at this pointed and figured it was time to start sharing.

What did you expect to see?

No memory leaks on Linux also.

@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 16, 2025
@bradfitz
Copy link
Contributor Author

Actually, I'm going to close this for now until there's something actionable. I'll reopen if/when I have a repro.

@mknyszek
Copy link
Contributor

up to 2.7 seconds now

Tangentially related, but we should probably remove the loop and switch to runtime.AddCleanup. I don't think that'll fix this issue though.

@bradfitz
Copy link
Contributor Author

I played with hacking away at the server, trying to minimize the real code down to minimal repro instead of building up a minimal repro from scratch, with the goal to eventually plug in a in-memory generated random data source rather than reading from the network.

That seemed to have worked.... at a certain point, Linux started reclaiming a small amount of memory on a test Linux box I was running. And over night it did one huge unique.Handle reclaim during one of its GCs, but zero for all the others throughout the night.

Now I need to binary search my deletions and see what the key deletion was.

In any case, it still reclaims super slowly on Linux even after my minimization.

Will report back when I have more.

@bradfitz
Copy link
Contributor Author

Okay, I think I've narrowed it down and figured out the macOS-vs-Linux difference: this is about the number goroutines producing new unique.Handles overwhelming the sole cleanup goroutine. My Mac laptop (M4 Max) is just significantly beefier than my dinky Linux VMs (both prod & dev).

Now I understand why my ground-up repro didn't work before: it was using a single goroutine.

I have a standalone repro now from the full server that doesn't depend on the network (using 100 goroutines adding+removing keys) but now that I think I know what's happening, I think I'll switch back to my bottom-up repro, slapping on some goroutines.

So @mknyszek, maybe runtime.AddCleanup is indeed the answer here.

@bradfitz bradfitz reopened this Feb 16, 2025
@bradfitz bradfitz changed the title unique: Handle memory leaks on Linux (but not macOS) unique: Handle memory leaks Feb 16, 2025
@bradfitz
Copy link
Contributor Author

bradfitz commented Feb 16, 2025

Here ya go: https://go.dev/play/p/rrNRBh6g-WF

Ignore that that's a playground link and also that it's written as a test function. Just run it locally (on either macOS or Linux) and watch it leak.

Running WithoutUnique, it doesn't grow:

% GODEBUG=gctrace=1 go test -v -run=WithoutUnique
...
gc 35 @27.366s 0%: 0.13+54+0.10 ms clock, 2.2+1.7/134/0+1.7 ms cpu, 3589->3667->1814 MB, 3715 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1500000; 3660669760 HeapAlloc, 2440.45 per item
gc 36 @28.590s 0%: 0.20+31+0.10 ms clock, 3.2+3.3/121/0+1.7 ms cpu, 3501->3544->1780 MB, 3629 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499999; 3702783968 HeapAlloc, 2468.52 per item
gc 37 @29.773s 0%: 0.12+36+0.038 ms clock, 1.9+2.8/135/0+0.61 ms cpu, 3433->3483->1790 MB, 3562 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499999; 3750955328 HeapAlloc, 2500.64 per item
    uniqueleak_test.go:124: # items = 1499984; 3678439048 HeapAlloc, 2452.32 per item
gc 38 @30.970s 0%: 0.18+50+0.11 ms clock, 2.8+3.2/128/0+1.8 ms cpu, 3451->3520->1810 MB, 3581 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499999; 3622019400 HeapAlloc, 2414.68 per item
gc 39 @32.194s 0%: 0.21+52+0.17 ms clock, 3.4+2.1/144/0+2.8 ms cpu, 3491->3563->1816 MB, 3622 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1500000; 3665755200 HeapAlloc, 2443.84 per item
gc 40 @33.458s 0%: 0.10+64+0.060 ms clock, 1.6+3.2/128/0+0.97 ms cpu, 3551->3642->1837 MB, 3633 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1500000; 3755686688 HeapAlloc, 2503.79 per item
gc 41 @34.742s 0%: 0.20+52+0.039 ms clock, 3.2+3.0/135/0+0.62 ms cpu, 3582->3655->1821 MB, 3675 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1500000; 3832429984 HeapAlloc, 2554.95 per item

But using unique, it leaks (with my hacked up unique package doing extra logging, as linked above)

% GODEBUG=gctrace=1 go test -v -run=WithUnique
gc 20 @17.533s 5%: 0.16+1023+0.010 ms clock, 2.6+501/4094/8.2+0.16 ms cpu, 6973->8495->4847 MB, 8189 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499986; 8882974072 HeapAlloc, 5922.04 per item
    uniqueleak_test.go:124: # items = 1499999; 8880822872 HeapAlloc, 5920.55 per item
    uniqueleak_test.go:124: # items = 1499996; 8541901672 HeapAlloc, 5694.62 per item
    uniqueleak_test.go:124: # items = 1499983; 8328100968 HeapAlloc, 5552.13 per item
    uniqueleak_test.go:124: # items = 1499986; 9557681720 HeapAlloc, 6371.85 per item
gc 21 @22.366s 5%: 0.16+1267+0.010 ms clock, 2.5+802/5066/14+0.17 ms cpu, 8255->10094->5671 MB, 9694 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499980; 10552296616 HeapAlloc, 7034.96 per item
    uniqueleak_test.go:124: # items = 1499996; 10549919064 HeapAlloc, 7033.30 per item
    uniqueleak_test.go:124: # items = 1499987; 10354593096 HeapAlloc, 6903.12 per item
    uniqueleak_test.go:124: # items = 1499997; 9738898648 HeapAlloc, 6492.61 per item
    uniqueleak_test.go:124: # items = 1499993; 10020571944 HeapAlloc, 6680.41 per item
    uniqueleak_test.go:124: # items = 1499984; 11420736632 HeapAlloc, 7613.91 per item
XXX did unique cleanup loop:  7469606 items 3946215 deleted 3523391 kept; took  26.446266s
XXX starting unique cleanup loop
gc 22 @28.087s 6%: 0.14+1478+0.010 ms clock, 2.3+1673/5913/9.8+0.17 ms cpu, 9659->11751->6463 MB, 11343 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499988; 12283776280 HeapAlloc, 8189.25 per item
    uniqueleak_test.go:124: # items = 1499997; 12282524312 HeapAlloc, 8188.37 per item
    uniqueleak_test.go:124: # items = 1499818; 12138670280 HeapAlloc, 8093.43 per item
    uniqueleak_test.go:124: # items = 1499987; 11612830136 HeapAlloc, 7741.95 per item
    uniqueleak_test.go:124: # items = 1499982; 10844047640 HeapAlloc, 7229.45 per item
    uniqueleak_test.go:124: # items = 1499975; 11746102088 HeapAlloc, 7830.87 per item
    uniqueleak_test.go:124: # items = 1499997; 13196850496 HeapAlloc, 8797.92 per item
gc 23 @34.794s 6%: 0.11+1706+0.013 ms clock, 1.8+2438/6823/4.2+0.20 ms cpu, 11008->13350->7317 MB, 12927 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499989; 13948964920 HeapAlloc, 9299.38 per item
    uniqueleak_test.go:124: # items = 1499989; 13947515720 HeapAlloc, 9298.41 per item
    uniqueleak_test.go:124: # items = 1499946; 13924360552 HeapAlloc, 9283.24 per item
    uniqueleak_test.go:124: # items = 1499997; 13429541096 HeapAlloc, 8953.05 per item
    uniqueleak_test.go:124: # items = 1499987; 12570533544 HeapAlloc, 8380.43 per item
    uniqueleak_test.go:124: # items = 1499985; 12730948488 HeapAlloc, 8487.38 per item
    uniqueleak_test.go:124: # items = 1499980; 13995042328 HeapAlloc, 9330.15 per item
    uniqueleak_test.go:124: # items = 1499991; 15426806832 HeapAlloc, 10284.60 per item
gc 24 @42.260s 7%: 0.14+1943+0.013 ms clock, 2.2+3785/7772/4.1+0.21 ms cpu, 12462->15056->8208 MB, 14635 MB goal, 0 MB stacks, 0 MB globals, 16 P
    uniqueleak_test.go:124: # items = 1499988; 15727126728 HeapAlloc, 10484.84 per item
    uniqueleak_test.go:124: # items = 1499995; 15726441208 HeapAlloc, 10484.33 per item
    uniqueleak_test.go:124: # items = 1499995; 15724350536 HeapAlloc, 10482.94 per item

Note that there are multiples GCs happening while the unique cleanup func is still running.

@bradfitz bradfitz changed the title unique: Handle memory leaks unique: Handle memory leaks; cleanup goroutine can't keep up Feb 16, 2025
@bradfitz
Copy link
Contributor Author

Yeah, AddCleanup seems to do the trick. But it's not enough by itself. It also needs a bit of back pressure. Here's a trashy fix:

https://gist.github.com/bradfitz/a136006f112fee47ce1d87232e779d24

(I've signed the CLA; consider that trashy patch safe to look at 😅)

@bradfitz
Copy link
Contributor Author

A potential fix for Go 1.23 (without using Go 1.24's AddCleanup) is https://gist.github.com/bradfitz/901f14f9c6dfd5c9344673a2c9478932 , making new handles wait for any ongoing cleanup to finish, but it's a bit too aggressive: it makes all goroutines wait.

But it works. 🤷‍♂

@mknyszek
Copy link
Contributor

Hm... We should make the unique cleanup more efficient by using runtime.AddCleanup (we can use finalizers in Go 1.23, maybe), but (just musing here) the fact that that's not enough and it still needs some kind of back-pressure mechanism makes me wonder if maybe any kind of canonicalization map just isn't worth it for this workload?

In other words, if there are so many new entries being created that deletion can't keep up, that suggests there isn't really all that much deduplication happening; that there are more inserts than lookups.

... But also maybe I'm wrong about that. Maybe there's enough of a benefit even if you only get, for example, 1 lookup on average after an insert, before a deletion. I'd love to understand this better if you can collect any metrics from production in an ad-hoc way, @bradfitz. And perhaps we need diagnostics (runtime/metrics, probably) to help people understand whether unique is actually working for them.

Off the top of my head, I'm not sure what the right back-pressure policy would be here. An alternative would be is that we could scale up deletions instead. Deletions on independent parts of the tree (especially for a big tree) should scale well in my testing. The runtime.AddCleanup cleanup function could put work on a queue instead of doing the deletion, and a pool of goroutines could consume this queue instead of just a single goroutine. We can scale this pool according to the rate of insertions (or even just always have enough goroutines on-hand to handle the load). There's a sort of natural back-pressure built-in as you start spending CPU time on these background cleaners, and the scheduler handles distribution of resources.

@mknyszek
Copy link
Contributor

Thank you for the thorough investigation @bradfitz!!

@bradfitz
Copy link
Contributor Author

maybe any kind of canonicalization map just isn't worth it for this workload

While that's likely true (we just did this as an experiment to see if it'd save the expected few hundred MB of heap), I strongly feel that it shouldn't be possible to use a Go API and see runaway memory growth. That is, users should be able to evaluate whether unique is right for them based on their measured CPU usage, memory usage, application throughput, etc, and not via OOMs. :) Even if unique isn't technically part of Go's GC, it reflects poorly on Go as a GC'ed language that it can't keep up with what seems like just normal garbage creation from a user's point of view. So I think back pressure is important in some form here (whether it's just blocking on making new Handles, or outcompeting application goroutines with more delete goroutines/delete work, etc) in order to make the user's measured throughput decrease, CPU increase, etc, and help them decide that unique was maybe the wrong choice. With such back pressure, we wouldn't necessarily need new metrics... a normal Benchmark func or throughput metrics would be enough.

@mknyszek
Copy link
Contributor

mknyszek commented Feb 17, 2025

Yes, totally agree. My apologies, I did not intend to suggest we should leave this problem alone.

Mostly I just wanted to understand your workload better. unique will always have some overhead, but by deduplicating memory, you save on both memory and GC costs, and ideally, at some point in your amount of deduplication, you break even and then do better. What I wanted to learn more about is the break-even point, and whether we consider the current one acceptable, using your app as a data point. :)

I plan to work on this this week; it shouldn't take too long to fix. While I'm at it I have some ideas to reduce the memory overhead of each entry in the global map, since using runtime.AddCleanup will increase it. (And that might also fix another issue I have in mind.) runtime.AddCleanup also takes a global lock that it probably shouldn't if we're going to use it in on a critical path like this.

(As an aside, yet another back-pressure idea I have is to let callers into unique.Make participate in the deletion work queue.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650256 mentions this issue: unique: use a bespoke concurrent weak set and runtime.AddCleanup

@mknyszek
Copy link
Contributor

CL 650256 is step one. We still need a back-pressure mechanism, and I'd like to make runtime.AddCleanup scale a little bit better, but it's a start.

@mknyszek mknyszek self-assigned this Feb 18, 2025
@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 18, 2025
@mknyszek mknyszek added this to the Go1.25 milestone Feb 18, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/650697 mentions this issue: runtime: schedule cleanups across multiple goroutines

@mknyszek
Copy link
Contributor

https://go.dev/cl/650697 (and changes beneath it) automatically scales up to GOMAXPROCS background cleanup goroutines. There's no explicit backpressure, only implicit backpressure through the scheduling of these CPU-bound cleanup goroutines. It appears successfully to cause TestWithUnique to level off, though at a higher amount of memory than TestWithoutUnique.

@bradfitz if you're willing to give it a shot in production, I'd be curious to know if it's good enough.

Applying real backpressure in runtime.AddCleanup seems to be trickier than we expected, because it's not safe to actually run cleanups directly from runtime.AddCleanup, due to the possibility of deadlocks (#71825 (comment)). We can force runtime.AddCleanup to block, maybe, but I'm still not really sure what a good policy would be.

@bradfitz
Copy link
Contributor Author

@bradfitz if you're willing to give it a shot in production, I'd be curious to know if it's good enough.

I'm traveling this week and getting very little computer time but I'll get back to this next week. I'm excited about all the changes! 🙏

@bradfitz
Copy link
Contributor Author

@mknyszek, results from prod, from a Go built at 486a06e3770a2331ae4141fd9f1197372075fbc2 (PS 14 of https://go-review.googlesource.com/c/go/+/650697)

commit 486a06e3770a2331ae4141fd9f1197372075fbc2 (HEAD -> change-650697)
Author: Michael Anthony Knyszek <mknyszek@google.com>
Date:   Wed Feb 19 16:33:21 2025 +0000

    runtime: schedule cleanups across multiple goroutines

It's better than before, but not perfect. Over the past 7 days:

Image

Green is not using unique. The yellow instance is the test, using unique at 486a06e3770a23.

Let me know if I can run any other experiments.

@mknyszek
Copy link
Contributor

Those results roughly reflect what I saw in the reproducer, which is good at least in terms of the reproducer being accurate. I'm curious to know if memory continues to grow further if left alone. I acknowledge there's a trend here over several days, and maybe this is just wishful thinking, but it kinda sorta looks like it's leveling out? Maybe?

If the leak problem is mostly fixed and it's just a new higher memory footprint, there might not be a way to make it perfect. unique does have overheads, the same way that putting a whole bunch of, uh, unique items into a map would have the overhead of the map.

Still, applying backpressure would probably make this all more stable. Still not sure what the right backpressure mechanism is here.

Simplest policy: each unique.Make needs to delete one stale entry? Seems fine, but we need to (1) identify that work outside of the cleanups and (2) synchronize with the cleanups somehow. Cleanups are a difficult place to apply backpressure because we can't safely run cleanups from AddCleanup. We can run the cleanup for unique from unique.Make, because we know exactly what the cleanup is going to do. So maybe this needs to happen inside the unique package...

@mknyszek
Copy link
Contributor

mknyszek commented Mar 10, 2025

I will get back to you once I have a clearer idea of what else to try, but these patches seem like a good place to start.

@bradfitz
Copy link
Contributor Author

I'll keep it running and get back to you with what the shape looks like in a few days.

@bradfitz
Copy link
Contributor Author

I'll keep it running and get back to you with what the shape looks like in a few days.

As promised:

Image

It's still going up it seems. The shape of the graph is quite ... unique.

@mknyszek
Copy link
Contributor

OK, so problem definitely not solved. Thank you for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants