xsnap leaks memory during snapshot write

[warner]

I was having trouble reproducing #5910 locally, by replaying a transcript extracted from the original slogfile. I realized that one thing the original was doing, but my replay was not, was writing out the periodic heap snapshot (every 200 deliveries or so). When I added that to packages/SwingSet/misc-tools/replay-transcript.js, I saw an immediate growth of the xsnap process' VmRSS size, about 5MB for every snapshot it wrote.

I'll dive into xsnap.c next, but I'm guessing that a temporary buffer is allocated to write a snapshot, and then it is never freed.

[warner]

My test program (https://github.com/Agoric/agoric-sdk/blob/614c849d61f8a2c24c82fbc48fcab29338774f25/packages/xsnap/test-snapshot-memleak.js) shows a memory leak of a trivial worker (just lockdown and our supervisor bundle) of about 30 kB per snapshot write. The leak is unaffected by putting large strings into the worker's heap, but adding new properties to the global object increases each leak by about 24 bytes. This suggests that the snapshot writing process is malloc()ing a temporary buffer based on the number of slots in use (not chunks, which hold strings), and then not freeing the buffer afterwards.

That makes the leak roughly proportional to the complexity of the heap (number of objects, properties, edges between objects, etc). Zoe is one of our more complicated vats, so the snapshots include a lot more slots, so the leak is faster.

I've let the Moddable folks know.

For now, our workaround of evicting each worker after each snapshot write is a decent one. If we really wanted to, we could claw back some performance by only evicting after every N snapshots, but I think it's better to wait for the upstream fix.

[warner]

The XS code does a really good job of not allocating memory. And I don't see anything obvious in the xsnap-worker.c code that might accumulate data created during the snapshot write.

I do see that the fxWriteSnapshot() function wraps the code in a mxTry/mxCatch macro pair, and I think those use longjmp, and the code is making a lot of subroutine calls, perhaps enough to grow the stack by a non-trivial degree. Perhaps the longjmp behavior is somehow creating an alternate stack and then leaking it? That's not how I know stacks to work, but I'm not seeing any obvious explanation for the leak yet.

[phoddie]

Here's a patch to xsSnapshot.c from Patrick (also provided by email) to use until we get the next Moddable SDK update pushed. (The leak was introduced as part of the recent work to fix snapshot divergences).

@@ -2035,6 +2035,8 @@ int fxWriteSnapshot(txMachine* the, txSnapshot* snapshot)
mxCatch(the) {

}
+ if (snapshot->slots)
+ c_free(snapshot->slots);

projectionAddress = &(snapshot->firstProjection);
while ((projection = *projectionAddress)) {

[warner]

Awesome. I've confirmed that fixes the leak.

[warner]

Next step: wait for Moddable to update the SDK, then make an https://github.com/agoric-labs/moddable PR to pull that in, then make an agoric-sdk PR to update our packages/xsnap/moddable submodule to that version.

[dckc]

I'm inclined to close this, since the symptoms are addressed by #5987 .

The open issue now is some technical debt: we're out of sync with moddable master. We were keeping this open until that was addresses, presuming it would be straightforward; but #6011 turned out to be not straightforward. Do we want to track "we're out of sync with moddable" as an issue? @warner @kriskowal @mhofman ?

[dckc]

I got a second from @mhofman and no objections on closing this.