nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.63k stars 29.61k forks source link

nodejs addon memory leak. Maybe it is the problem of gc. #21441

Closed caijw closed 5 years ago

caijw commented 6 years ago

I found that when writing nodejs addon to produce Buffer, memory leak will happen. My test code is : https://github.com/caijw/test/tree/master/bufferLeak

Run the index.js in the bufferLeak folder. I found that the memory used in my pc grows and won't decrease. before running: image

after generate 250 buffers object:

image

and it keeps on consuming my pc's memory util i shut down this program.

killagu commented 6 years ago

@caijw You can use the --trace-gc to see did gc occurred and process.memoryUsage() to see memory usage.

I run the index.js , the gc works well.

Before gc the memory usage is

{ rss: 213196800,
  heapTotal: 7708672,
  heapUsed: 5258688,
  external: 10272 }

And after gc the memory usage is

{ rss: 73826304,
  heapTotal: 7184384,
  heapUsed: 4721800,
  external: 8320 }
ChALkeR commented 6 years ago

https://github.com/caijw/test/blob/master/bufferLeak/binding.cpp#L16-L30 Is theCopy you malloc ever freed?

See https://nodejs.org/api/n-api.html#n_api_napi_create_external_buffer for documentation.

killagu commented 6 years ago

The freeData work on node v8 and not work v10.

killagu commented 6 years ago

If call global.gc(), the memory will be collected. It looks like gc is not triggered.

killagu commented 6 years ago

If alloc a large buffer in the loop, like Buffer.alloc(1024*1024) gc will be triggered too.

But if alloc a small buffer in the loop, like Buffer.alloc(1024) the gc will not be trigger.

Is it a bug?

mhdawson commented 6 years ago

From what I can tell

1) Only a small amount of memory is being allocated on the heap (the object associated with the buffer) 2) A large amount of native memory is allocated and associated with the object in the heap 3) Since there is only a small amount of memory in the heap, there is no reason for a gc to occur. 4) Invoking a gc frees the memory (from what I read above).

This is a classic problem with keeping native memory alive through heap references. Its difficult to address as the runtime does not have a good way to know when a gc is required.

You can try to add a call to

NAPI_EXTERN napi_status napi_adjust_external_memory(napi_env env,
                                                    int64_t change_in_bytes,
                                                    int64_t* adjusted_value);

to give the garbage collector a hit by YMMV across implementations.

Instead, I'd suggest trying to free the buffer once it is no longer needed as opposed to waiting for the gc to collect the object.

killagu commented 6 years ago

@mhdawson Thank you for the answer. But I still have two problems.

  1. Buffer.alloc allocate memory use the native memory, whats the difference between Buffer.alloc(1024) and Buffer.alloc(1024 * 1024)?
  2. If they are same, why alloc a larger buffer can trigger gc?
mhdawson commented 6 years ago

@killagu I'm not sure on the 1024 versus the 1024*1024 as it is an implementation detail as to when a gc decides to do a gc (ie not part of the spec). My guess is that b8 uses a large allocation a trigger for doing a gc. @hashseed do you have any info you can add?

caijw commented 6 years ago

@mhdawson I use nodejs v9.0.0 to run this addon, it performs well. The gc will trigger and free theCopy. There might be some problem with the gc of nodejs v10. Although the heap is small ,but the nodejs v9.0.0 do perform well and gc triggers.

bnoordhuis commented 6 years ago

Call napi_adjust_external_memory() after creating the buffer and before freeing the memory, that gives the GC a more accurate view of memory consumption.

@nodejs/n-api Perhaps napi_create_external_buffer() should do that automatically. It knows the size.

killagu commented 6 years ago

@bnoordhuis Thank you for the reply. Call the napi_adjust_external_memory works well.

gabrielschulhof commented 6 years ago

@bnoordhuis how about moving the integration up into node::Buffer::New()?

gabrielschulhof commented 6 years ago

@bnoordhuis that way all addons benefit, not just N-API ones.

caijw commented 6 years ago

@mhdawson @bnoordhuis, thanks a lot. Calling napi_adjust_external_memory() do work well, maybe it is the feature of v8 memory manage.But when i call napi_create_external_buffer(), i have give nodejs a hint of the size of allocated native memory. Calling napi_adjust_external_memory() after napi_create_external_buffer() may be redundant grammatically.

bnoordhuis commented 6 years ago

@gabrielschulhof Buffer::New() does not always know the size of the buffer. The overload that n-api calls takes a pointer but not a size. It's just going to be confusing when some overloads call AdjustAmountOfExternalAllocatedMemory() but others don't.

gabrielschulhof commented 6 years ago

@bnoordhuis if we sometimes call AdjustAmountOfExternalAllocatedMemory() on behalf of the addon and sometimes not, will that not, in the long run, distort V8's picture of the actual used memory, especially since we've just advised that the addon developer make such a call. The reason for this, in my thinking, is that some addons already call AdjustAmountOfExternalAllocatedMemory() whereas others do not, and so, for the former, we'd end up double-counting.

gabrielschulhof commented 5 years ago

@caijw hopefully this has addressed your concerns. If you have any more trouble, please re-open this issue or open another one.