Geoff Goodman (Feb 15 2024 at 17:08): Geoff Goodman (Feb 15 2024 at 17:08):I tried doing a simple POC with the golang wasmtime bindings. The idea was that I would isolate a workload on a per-request basis. To accomplish this, I had a single engine and was creating per-request stores, linkers and instances. The instances were based on a pre-compiled WASM module.
I noticed that I quickly OOM'd. Also didn't see obvious ways to explicitly free up those ephemeral objects.
My guess is that I'm probably holding it wrong but wanted to know if others have seen such issues and how they got around them.
Joel Dice (Feb 15 2024 at 17:29):We do per-request isolation in Spin using InstancePre to make a new Instance (with a new Store) for each request with a variety of languages (including (Tiny)Go) and haven't had such issues. Can you verify that you're definitely dropping the instance and store after each request?
Geoff Goodman (Feb 15 2024 at 17:51):@Joel Dice I'm embedding wasmtime in a go project, not running go as a guest language. I'm NOT explicitly dropping the instance and store because I couldn't find any way to do so.
Geoff Goodman (Feb 15 2024 at 17:53):It might be a gap in the bindings or just some trickery with Contexts that I didn't figure out.
Joel Dice (Feb 15 2024 at 17:54):Ah, I see. Yeah, I don't have experience with wasmtime-go myself. I'm guessing it's based on Wasmtime's C bindings, which presumably has a way to dispose of such resources. Might be worth looking through the wasmtime-go source code and/or opening an issue on that repo.
Geoff Goodman (Feb 15 2024 at 17:55):Ah, let me see if I can find out the c api.
Geoff Goodman (Feb 15 2024 at 17:57):OK, found them so it looks like the c bindings don't expose an explicit drop() or free() method. It seems like they expect a finalizer. Is that like a destructor?
Lann Martin (Feb 15 2024 at 17:58):A finalizer is a Go runtime feature that frees resources once a particular object gets garbage collected
Lann Martin (Feb 15 2024 at 17:59):So you'd need to remove any references to the store, including any "child" objects that might refer to it
Geoff Goodman (Feb 15 2024 at 18:02):In my little POC, the only long-lived references are these:
engine := wasmtime.NewEngine()
mod, err := wasmtime.NewModule(engine, quickjsWasmBytes)
if err != nil {
slog.Error("Error instantiating module: %e", err)
os.Exit(1)
}
Beyond that, everything is assigned in the request handler and _should_ only have 1-way references back to the shared engine.
Perhaps I'm making a mistake w/ per-request linkers?
Joel Dice (Feb 15 2024 at 18:02):@Lann Martin But it's not an reference-counting GC, is it? So it won't be collected until there's Go-visible memory pressure, which C and Rust allocations won't necessarily contribute to. I think we'll want to have a pro-active way of disposing these things without leaning on the GC.
Lann Martin (Feb 15 2024 at 18:03):You'd hope GC would run before OOM but yeah it isn't deterministic
Lann Martin (Feb 15 2024 at 18:03):agree there should probably be a way to explicitly deallocate
Geoff Goodman (Feb 15 2024 at 18:04):I think the idiomatic trick in go would be:
store := wasmtime.NewStore(engine)
defer store.Drop()
I can almost guarantee it won't run before OOM if it thinks each of these Instance and Store objects are tiny and doesn't even see the C/Rust objects they point to as being part of the heap it is managing.
Lann Martin (Feb 15 2024 at 18:05):@Geoff Goodman You'd maybe call it store.Close() in go, but yes
Geoff Goodman (Feb 15 2024 at 18:05):I'm neither a rustacean nor gopher so I defer to the experts :D
Geoff Goodman (Feb 15 2024 at 18:09):How does that passed-in finalizer work in the c bindings?
Geoff Goodman (Feb 15 2024 at 18:10):#[no_mangle]
pub extern "C" fn wasmtime_store_new(
engine: &wasm_engine_t,
data: *mut c_void,
finalizer: Option<extern "C" fn(*mut c_void)>,
) -> Box<wasmtime_store_t> {
For reference: https://github.com/bytecodealliance/wasmtime/blob/83a5a1a607f5609249e451cbadd38827140bb7eb/crates/c-api/src/store.rs#L88-L93
Lann Martin (Feb 15 2024 at 18:10):finalizers are a feature of the Go garbage collector; they have nothing to do with wasmtime or C
Joel Dice (Feb 15 2024 at 18:11):Looks like he's referring to a Wasmtime C API thing independent of Go, though
Lann Martin (Feb 15 2024 at 18:11):ohh sorry; maybe an unfortunate terminology collision?
Joel Dice (Feb 15 2024 at 18:12):Looks to me like the finalizer param is in case the data param needs to be disposed with custom code supplied by the embedder
Geoff Goodman (Feb 15 2024 at 18:13):If someone is using the c bindings, how does one drop a store?
Lann Martin (Feb 15 2024 at 18:14):wasmtime_store_delete: https://github.com/bytecodealliance/wasmtime-go/blob/v1.0.0/store.go#L72
Geoff Goodman (Feb 15 2024 at 18:15):Nice. Missed that in the header. Seems like that offers a nice path to opt into explicit resource management.
Lann Martin (Feb 15 2024 at 18:16):It would probably require some more changes to the Go API to coexist with the Go finalizer
Joel Dice (Feb 15 2024 at 18:16):You'll want to set store._ptr to null after you delete it (and not call wasmtime_store_delete if it's already null) so it doesn't get deleted twice in the finalizer.
Joel Dice (Feb 15 2024 at 18:17):And check for null in all the methods.
Lann Martin (Feb 15 2024 at 18:17):yeah really needs to be an issue on the wasmtime-go repo for discussion
Geoff Goodman (Feb 15 2024 at 18:17):I can kick that off.
Lann Martin (Feb 15 2024 at 18:17):Thanks!
Alex Crichton (Feb 15 2024 at 18:17):When I originally wrote the Go bindings I was hoping to lean heavily on the Go GC to clean things up to avoid the need for .Drop() everywhere since sometimes the lifetimes can be pretty tricky, but I do agree that we should add an explicit Drop method where needed!
Is it possible to get a snapshot of the heap when OOM was triggered? There may also be a bug in the wasmtime-go bindings that is leaking something by accident
Alex Crichton (Feb 15 2024 at 18:18):(also agreed an issue would be good to track as well)
Joel Dice (Feb 15 2024 at 18:26):To reiterate (since this is kind of a pet peeve of mine): whenever a GC-allocated object points to resources outside the GC-managed heap (e.g. sockets, malloc'd buffers, graphics contexts, file descriptors, mmap'd regions, etc.) and the only way to clean them up is via GC-triggered finalizers, you're going to have a bad time eventually. The GC doesn't see any of those external resources as contributing to memory pressure for the GC-managed heap, so you can easily run out of those resources before the next GC cycle.
Which isn't to criticize use of finalizers to dispose of such things as a good (and ergonomic) first step, but there's a reason why e.g. Python got a with statement, Java got try-with-resources, and Go has defer -- they all acknowledge that some resources need to be cleaned up independently of GC.
Geoff Goodman (Feb 15 2024 at 18:27):Even JavaScript is getting using and await using.
Lann Martin (Feb 15 2024 at 18:29):fwiw, using finalizers is quite rare in Go; people expect to manually clean up unmanaged resources
Geoff Goodman (Feb 15 2024 at 18:37):Issue created, thanks for the insight folks: https://github.com/bytecodealliance/wasmtime-go/issues/209
Alex Crichton (Feb 15 2024 at 18:39):This is a bit orthogonal at this point, but are y'all saying that wasmtime-go shouldn't have any finalizers at all? I agree it should have the option to explicitly finalize, but it'd be surprising to me if it's recommended to remove all finalizers
Alex Crichton (Feb 15 2024 at 18:40):insofar as I'd expect a managed language like Go to not have memory leaks by default, but if finalizers were removed then the wasmtime bindings would have memory leaks by default
Lann Martin (Feb 15 2024 at 18:40):go finalizers have pretty weak guarantees, even in practice
Joel Dice (Feb 15 2024 at 18:41):I don't know what's idiomatic in Go, so no opinion on that from me, but you could equally say the C ABI has memory leaks by default (i.e. you have to explicitly call wasmtime_store_delete to avoid them).
Alex Crichton (Feb 15 2024 at 18:41):Well no one expects C to manage memory for you, but I figured that users of Python and Go for example expect to not have memory leaks by default
Alex Crichton (Feb 15 2024 at 18:41):e.g. if I open a file in Python I'd expect that it'd eventually get closed when I stop referring to it
Alex Crichton (Feb 15 2024 at 18:42):like I understand nothing is deterministic about finalizers and it's a good point that finalizers hide the "true cost" of an object, but despite all that I was always under the impression that finalizers are exactly intended for this sort of use case of C ffi bindings cleanup
Joel Dice (Feb 15 2024 at 18:42):Python is interesting because it has a reference-counting+plus-cycle-collector GC, so it tends to be more deterministic in practice (and thus maybe lean more heavily on finalizers).
Alex Crichton (Feb 15 2024 at 18:42):(again not saying explicit finalizers shouldn't exist, that unambiguously is a good idea)
Alex Crichton (Feb 15 2024 at 18:43):I'm also asking b/c this would be a major change to the python/go bindings to remove finalizers
Joel Dice (Feb 15 2024 at 18:43):FWIW, I think using finalizers as "a backup" for explicit disposal makes sense, and is common in Java at least. I just don't know what's right for Go.
Alex Crichton (Feb 15 2024 at 18:43):(I understand that the issue isn't saying remove finalizers, so talking about something somewhat orthogonal now)
Joel Dice (Feb 15 2024 at 18:44):Another option would be to print a warning in the finalizer.
Joel Dice (Feb 15 2024 at 18:44):Which I've seen in Python.
Alex Crichton (Feb 15 2024 at 18:44):yeah that makes sense to me, but it from
fwiw, using finalizers is quite rare in Go; people expect to manually clean up unmanaged resources
it sounded like @Lann Martin was saying we should remove the current finalizers
Alex Crichton (Feb 15 2024 at 18:44):oh interesting (about warnings)
Lann Martin (Feb 15 2024 at 18:45):that was more a response to @Joel Dice's micro-rant :smile:
Lann Martin (Feb 15 2024 at 18:46):I would agree that using a finalizer as backup to an explicit close makes sense
Joel Dice (Feb 15 2024 at 18:46):We should print a warning in the finalizers that includes a link to my micro-rant on Zulip.
Alex Crichton (Feb 15 2024 at 18:46):can I include your email in the warning?
Alex Crichton (Feb 15 2024 at 18:47):thanks Geoff for opening https://github.com/bytecodealliance/wasmtime-go/issues/209! would you be interested in helping to implement that? I'd be happy to review!
It also sounds like we should mirror that in the wasmtime-py Python bindings
Joel Dice (Feb 15 2024 at 18:49):FWIW, on the Python guest side we make resource handles implement ContextManager, allowing them to be used in with statements. I'm guessing we'll want to do something similar on the host side.
Likewise, it might be worth looking at how the folks building guest tooling for Go are handling resource handles.
Joel Dice (Feb 15 2024 at 18:50):resource handle lifetimes really have to be managed explicitly due to parent-child relationships
Alex Crichton (Feb 15 2024 at 18:50):for sure yeah
Alex Crichton (Feb 15 2024 at 18:51):this really shows that someone other than me, who knows very little of Python and Go, should be primarily maintaining wasmtime-{go,py}
Joel Dice (Feb 15 2024 at 18:52):(the silence you're hearing is the sound of nobody volunteering)
Alex Crichton (Feb 15 2024 at 18:53):it's ok issues are enough of a signal for now :)
Geoff Goodman (Feb 15 2024 at 18:53):Sorry, in a 1:1
Geoff Goodman (Feb 15 2024 at 19:21):I'd be interested in the work but I can't realistically commit to the undertaking without setting folks (and myself) up for disappointment. I'm still in the exploratory phase in seeing if WASM might form a strategic investment for us. I'm personally very bullish about the tech but there are quite a few more stakeholders.
Joel Dice (Feb 15 2024 at 19:24):I didn't mean to imply that you should volunteer to be the new wasmtime-{go,py} maintainer, FWIW. What you're saying is totally reasonable.
Joel Dice (Feb 15 2024 at 19:25):I was just teasing Alex since he's maintainer of half the projects on the Internet.
Ralph (Feb 15 2024 at 19:26):only 40%
Guy Bedford (Feb 15 2024 at 20:03):
resourcehandle lifetimes really have to be managed explicitly due to parent-child relationships
FWIW in JS we got around this by having the finalizer function closure itself maintain a strong GC link to the parent resource, so the GC can still work despite these, as suggested by @Alex Crichton
Lann Martin (Feb 15 2024 at 20:17):This seems very tricky since dropping a resource is sometimes a positive signal in e.g. wasi-http
Guy Bedford (Feb 15 2024 at 20:20):(yeah, we support GC as the backup, with explicit resource management as the first line approach (ready for the soon to be shipped using)) I don't think there's any disagreement here just noting that GC isn't inhibited by child resource relations
Joel Dice (Feb 15 2024 at 21:11):Guy Bedford said:
FWIW in JS we got around this by having the finalizer function closure itself maintain a strong GC link to the parent resource, so the GC can still work despite these, as suggested by Alex Crichton
That's a cool trick. I should do that in componentize-py, too.
Geoff Goodman (Feb 16 2024 at 00:49):@Guy Bedford any chance you could point me to where you are employing that trick?
Mossaka (Joe) (Feb 20 2024 at 19:15):this really shows that someone other than me, who knows very little of Python and Go, should be primarily maintaining wasmtime-{go,py}
I will discuss this in the sig go subgroup to check interests for maintaining the wasmtime-go sdk.
Geoff Goodman (Feb 21 2024 at 21:38):@Alex Crichton a big thank you. Testing now.
Geoff Goodman (Feb 22 2024 at 00:48):No more memory exhaustion AFAICT. :thumbs_up:
Last updated: Nov 22 2024 at 16:03 UTC