Export the .release() functions so that user code can explicitly release memory.

[jeromegn]

What's the best way to free memory from various interactions with v8?

I noticed memory seemed to grow unbounded in a high concurrency scenario I'm running locally:

• Single Isolate
• 8 Contexts
• Each context stores a function to be called

I tried setting some *v8.Value to nil after I was done, hoping they would be finalized. Maybe there's a more explicit way of doing that?

Locally, I added some basic code for v8::HeapStatistics. My code uses accessors which I believe is pretty bad in terms of performance (going through the cgo membrane is "slow"). I'm sure it would be possible to expose it as a transferable C struct. The heap statistics object has a simple data structure.

I was also planning on playing with LowMemoryNotification and MemoryPressureNotification to try and force v8 to garbage collect.

[augustoroman]

Yeah, I've been thinking about this recently too. We need to have a Close or Dispose function on Value, Context, and Isolate that will cleanup and clear the finalizer manually. Use of the object after that will cause kabooms.

[jeromegn]

I've been playing with LowMemoryNotification when the heap gets too high. It mostly works, but then will segfault if there's too much concurrency.

For example:

context(9423,0x70000539f000) malloc: *** error for object 0x542bdb0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Received signal 6

==== C stack trace ===============================

 [0x000004ab6e34]
 [0x7fffccffab3a]
 [0x000000000000]
 [0x7fffcce7f420]
 [0x7fffccf6efe7]
 [0x000004653d16]
 [0x000004663e6e]
 [0x00000465eccb]
 [0x00000465e5f2]
 [0x0000046310cc]
 [0x00000462f604]
 [0x00000462eb47]
 [0x00000462eeb5]
 [0x0000042c6f9a]
 [0x0000042906ed]
 [0x000004056050]
[end of stack trace]
[9423:0x6800000]  1020081 ms: [Finished reentrant Mark-Compact during Mark-sweep.]


#
# Fatal error in ../src/heap/mark-compact.cc, line 3600
# Check failed: free_end > free_start.
#

==== C stack trace ===============================

 [0x000004ab6f73]
 [0x000004aba0db]
 [0x000004ab53ec]
 [0x00000466cde2]
 [0x00000466185f]
 [0x000004661b94]
 [0x0000046752ac]
 [0x000004abe779]
 [0x000004ab8747]
 [0x7fffcd00493b]
 [0x7fffcd004887]
 [0x7fffcd00408d]


#
# Fatal error in ../src/heap/mark-compact.cc, line 3639
# Check failed: p->area_end() > free_start.
#

==== C stack trace ===============================

 [0x000004ab6f73]
 [0x000004aba0db]
 [0x000004ab53ec]
 [0x00000466ce03]
 [0x00000466185f]
 [0x000004661b94]
 [0x0000046752cf]
 [0x000004abe779]
 [0x000004ab8747]
 [0x7fffcd00493b]
 [0x7fffcd004887]
 [0x7fffcd00408d]
signal: abort trap

Not sure how this happens, I can try and troubleshoot that.

[jeromegn]

Ok, I fixed my segfaults by using the ISOLATE_SCOPE macro, my bad for not using it before 🤦‍♂️

I also tried setting the Value's release method as Release and am manually calling that when I know I'm done with a value. I think it helps, but heap still grows uncontrollably for a simple example.

I'll keep looking into it.

[augustoroman]

Can you create a go test that demonstrates the heap growth?

[jeromegn]

This test creates a small leak (3.5MB -> 5MB): https://gist.github.com/jeromegn/39ddc09fb694ed9cf5d8293226bd5393

Of course v8's GC is not perfect, and this test is definitely imperfect: it rounds the floats (for the heap) heavily. It still shows an increase though.

My current thought is that the Context.Create() method should automatically clean itself up. It will create values and nested values which can't be released easily. The bigger I make the object in my test, the bigger the leak.

I would expect golang to GC these created values, but I'm not sure it's doing it.

That's only my current theory, maybe my test isn't meaningful. In the real world, I'm sending about 50K requests over 20 seconds, each request is "serialized" (header, url, method, no body) and created as a value, used as an argument for a function from v8. every 5 seconds, I check the heap, if it's over 32MB (arbitrary limit), I will trigger a GC on v8, which tends to clean up about 20-50% of the heap used, but it has its limits, it won't go lower than 50MB or so. The isolate initially uses 3.5MB of heap. I'm carefully releasing all values I created or got or sent to/from v8 when I'm done with them.

I've been able to save some heap in my test by releasing indexes and map keys when creating values. Both v (*Value) from here https://github.com/augustoroman/v8/blob/master/v8_create.go#L136 and here: https://github.com/augustoroman/v8/blob/master/v8_create.go#L168. AFAICT, these are not necessary to keep around.

Each value created from go is using v8::Persistent which tells v8 to keep the value around forever or until explicitly reset. Maybe we need to be able to create "local" values? I don't know enough about how v8 works to tell for sure. I think all values not explicitly created by users of this go package should probably not be persistent.

[augustoroman]

The v8::Persistent usage is ok: We do want v8 to keep the value around forever or until the go-side *v8.Value object is disposed of. Right now, though, that can only occur when the GC runs. That means that the two "leaked" values you found are indeed wasting memory until the GC runs, so immediately disposing of those is the right thing to do.

But that doesn't solve your problems, right? Sounds like we might be leaking some more somewhere else. Before triggering the GC on v8, it's probably worth it to call runtime.GC() a few times too to release as much of the un-referenced *v8.Value instances as possible. If that still doesn't fix your heap problems, then we have a real leak somewhere.

[augustoroman]

Well, with the extra v.release() calls from 425986c, it looks like the test you provided only increases the v8 memory by 0.5MB, regardless of the number of iterations. It also is substantially faster than relying on runtime.GC(). For this particular test, I think that the extra 0.5MB is related to v8 doing some code caching or something -- at least, that's my guess.

[augustoroman]

FYI, thanks for your test gist -- I've uploaded that to the lowmem_and_leaks branch. I think the HeapStatistics API you added should be added to master, but I'll need to doc it and test it properly first.

[jeromegn]

v8::Persistent is definitely the right wrapper for any code intentionally interacting with v8. I wouldn't use persistence for things that are not in control the user of this package. For example, Object keys: when casting a map[string]interface{} into v8, we're making values out of every map keys. These v8::Persistent<v8::Value> are not accessible for users of this package. They also tell v8 to keep them around forever and then we forget about them entirely.

Correct me if I'm wrong :) Just reading through the code and I think that's what's happening. Some of it could be avoided by not making all values persistent. Values used temporarily or "non-root" level (when calling context.Create(root interface{}) *v8.Value), should not be persistent. I assume v8 will figure it out and keep them around only for as long as need be (I'm not sure.)

Another way would be to also return all secondary values created by a ctx.Create() call. Maybe as a second return value, pushing error as a third return value, in the form of a []*v8.Value.

My test was a bit crude, in a real-world scenario, even if I aggressively release, the heap grows much more than 0.5MB (not evaluating new code.)

I do hope we can add HeapStatistics somewhere. It's insanely useful. Am I doing it right in my implementation? I didn't know if it was best to wrap it in something, but I read that fields from the C struct directly was faster somehow.

[augustoroman]

We don't make a new v8.Value for each map key, we pass the string key value directly to v8.

Also, each intermediate v8::Persistent that we create during v8.Create should now be cleaned up with 425986c -- try cherry-picking that changing into your code and seeing if it makes a significant difference.

In theory, we should only create a v8::Persistent when we're handing the value over to go to keep a reference to it and we should be releasing that persistent value when the go object is GC'd or explicitly released. v8.Create was previously creating a bunch of of persistent values and letting the GC clean them up.

However, there's still a leak somewhere. 78e1d15 shows that the unique strings are being kept around in v8 somehow.

As for HeapStatistics, yes I think you are doing it exactly right -- it's not much data to just copy the whole thing each time.

[jeromegn]

We don't make a new v8.Value for each map key, we pass the string key value directly to v8.

Ah, that makes sense, I wasn't sure how that worked exactly.

As for HeapStatistics, yes I think you are doing it exactly right -- it's not much data to just copy the whole thing each time.

I can clean it up, write some tests and create a PR.

[augustoroman]

re: HeapStatistics, sounds great, please send a PR. Grab all of the values out of the HeapStatistics returned by v8.

[jeromegn]

Ah, I implemented v8::HeapSnapshot::TakeHeapSnapshot() and saved to a file, then loaded in Chrome (well, Opera, but same thing) and compared heap of multiple snapshots.

The leak was coming from the go callback magic. I was binding a function on every request. These don't go away unfortunately. So instead, I made a generic function that's only bound once per context.

Meanwhile, I also implemented MemoryPressureNotification() on an Isolate. Very practical. In my use case, I'm setting and soft and hard memory limit. Every x seconds, I check the heap and send a memory pressure notification (can be done from a different thread, it's not blocking a thread to do so.)

I'll send a PR for that at some point, because why not.