src: fix memory leak for v8.serialize

[liuxingbaoyu]

Fixes: #40828
Refs: #38300

[addaleax]

#40828 (comment) is the right comment here here: This is currently not working because the garbage collector does not get a chance to run, including in the test case here.

This change here is not a good solution. It adds an unnecessary buffer copy without real benefit.

If you do want to fix this in a better way, you’ll need to modify Buffer::New(Environment*,char*,size_t) to not call into Buffer::New(Environment*,char*,size_t,FreeCallback,void); instead.

The best way to fix this is to keep reminding users that Buffer GC may need to perform some cleanup asynchronously, and Buffer instances may be kept alive until synchronous code stops running.

[liuxingbaoyu]

@addaleax

Thanks for your review!

This change here is not a good solution. It adds an unnecessary buffer copy without real benefit.

Although an extra memory copy is added, the test becomes faster, perhaps due to the reduced use of CleanupHook.

And it takes less memory and is more stable.

In some cases, such as babel, it can reduce memory by 300~500mb.

If you do want to fix this in a better way, you’ll need to modify Buffer::New(Environment*,char*,size_t) to not call into Buffer::New(Environment*,char*,size_t,FreeCallback,void); instead.

Can you give me more help? I'd love to use this better method!

I have tried modifying the CallbackInfo to release the buffer synchronously, but that requires a lock on the CleanupHook.

And using CleanupHook also seems to have an impact on performance.

The best way to fix this is to keep reminding users that Buffer GC may need to perform some cleanup asynchronously, and Buffer instances may be kept alive until synchronous code stops running.

But this problem leads to bad performance and even crashes, and there is no good way to avoid it other than changing everything to async.

[addaleax]

Can you give me more help? I'd love to use this better method!

The goal here would be to avoid CallbackInfo/CleanupHook entirely, and instead let V8 handle the allocation release. Passing a free callback to V8 is more efficient than passing one to the 5-argument Buffer::New() version, because the V8 callback makes fewer API guarantees (for example, it can run on any thread and can be called as part of GC; on the other hand, the Buffer::New() callback guarantees that JS can be executed from inside the callback, for legacy compatibility reasons).

So – the idea here is, instead of using Buffer::New(Environment*, char*, size_t, FreeCallback, void*) inside of Buffer::New(Environment*, char*, size_t):

• Creating a v8::BackingStore with a release callback using v8::ArrayBuffer::NewBackingStore(char*, size_t, DeleterCallback, void*)
• Creating an v8::ArrayBuffer from it using v8::ArrayBuffer::New(Isolate*, std::shared_ptr<BackingStore>)
• Create a Node.js buffer from that using Buffer::New(Environment*, Local<ArrayBuffer>, size_t, size_t)

The really nice thing about this approach would be that it takes care of this problem in all situations in which Buffer::New(Environment*, char*, size_t) is used, including addons which use it.

Although an extra memory copy is added, the test becomes faster, perhaps due to the reduced use of CleanupHook.

I assume that this would depend a bit on how much memory is allocated – longer buffers take longer to copy, but (typically) a constant amount of time to release.

In some cases, such as babel, it can reduce memory by 300~500mb.

Nice! To be clear, I would absolutely love to see this :)

[liuxingbaoyu]

Thank you very much for your detailed help, I will try it!

[addaleax]

Awesome, thank you!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43471/

[liuxingbaoyu]

It seems that the test is so strict that it fails on jenkins, I changed it from less than 10mb increments to less than double.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43516/

[nodejs-github-bot]

Landed in ce29d28