std::max(..., kMaxLength leads to massive GC pressure #72
Comments
|
I can, however, confirm that on Node.js 18, the |
Node.js 17/18 seem to have fixed the issue that led to 'random' corrupt memory in `node-ffi`, and if also applying a fix for incorrect usage of std::max instead of std::min (node-ffi-napi/ref-napi#72), we can finally get back to original performance for this flow *predictably*. In addition to that, we download our own `node.exe` and yarn CLI so one would not have to update globally to Node.js 18, breaking some other code (such as older webpack).
…his will result in Buffers being allocated to the size requested up to kMaxLength (0x3fffffff) instead of the current behavior of always allocating kMaxLength. Since these buffers are allocated in the node external memory (limited to 64M) the old behavior would very quickly cause massive GC pressure as the GC is triggered on each new Buffer allocation once the node external memory limit is reached. It appears the old behavior was not intentional and should have always been std::min(). This should fix node-ffi-napi#72
|
As others have observed when I tried to switch from ref (NaN) to ref-napi my code experienced significant performance degradation (functionally it worked fine). It took me a while to recognize this thread as the potential fix. Long story short I'm not sure the fixes applied by both the change to using 'std::' and the subsequent patch to account for the 'ERR_BUFFER_OUT_OF_BOUNDS' issue entirely resolve all issues. Or to put it a different way, I think they may introduce or expose a different one. My apologies for the length of this post but unfortunately it's not entirely clear to me what exactly is the problem but I can reliable demonstrate incorrect behavior. First note that I am using Docker containers and can reliably reproduce the failures using only ref-napi & ffi-napi (e.g. the failures aren't part of code I wrote). I start with the official nodejs v18 Docker images. I've tried both node:18-alpine and node:latest (Debian Buster). I cloned the ref-napi code & ffi-napi code repositories. I apply the suggested fixes to the ref-napi code & install it using '--build-from-source' in the Docker image. I then simply run the mocha tests (e.g. 'npm test'), note that for some reason I had to update the package.json for ref-napi to use '--v8-expose-gc' instead of the '--expose-gc' that was there to make the tests work with the gc, that can't be related to the issues in the main code but thought I'd note it. Running the tests will fail at the 'offset' test within 'pointer.js' & then do a core dump after the first successful 'ref/deref' test. This seemed extremely weird as there are other tests that simply write to a pointer & read back from the same pointer. I hate to admit it but I added a bunch of 'printf' statements to the ref-napi binding.cc to try to trace the problem. I was able to observe that for a call to 'readPointer' that has zero length (e.g. just write to a pointer & read from it) that the C++ code properly finds and dereferences the underlying pointer but the call to 'WrapPointer' results in a zero-length, e.g. no data ArrayBuffer. A previous test in the test suite that writes to a pointer & reads back from the pointer using a non-zero length is successfully. Based on this I naively decided to try adding the following line of code after the 'length = std::min<size_t>(length,kMaxLength) line, specifically: length = std::max<size_t>(length,1) Yes all that should do is if length == 0 it will make length = 1. In any case this FIXES the issue with ref-napi failing it's test suite. However I also use ffi-napi in my code & other investigation suggested that my 'fix' isn't really valid. This is already very long (my apologies) so I'll just note that running the ffi-napi test suite using ref-napi with the fixes suggested by this thread (e.g. std::min<size_t>(length,kMaxLength)) & my additional 'potential fix' of making sure length is at least of size 1 both result in ffi-napi failing its tests (at different places) & ultimately resulting in a core dump. To be clear the original fixes for the ref-napi performance issue result in ffi-napi failing it's test suite & my additional code doesn't help though they fail at different places in the testing. Again my apologies for the extremely long post but I thought it best to be descriptive about what I have done in testing the original fix to the performance issue. I can supply Docker files that can be used to perform the tests I ran. I also was able to get lldb to install in the node:18-alpine version so I can supply backtraces for the core dumps when running the ref-napi and/or ffi-napi test suites upon request. Unfortunately I have to get back to finishing my original project so while I'm happy to help do tests etc. I can't really spend more time investigating the nature of this issue and frankly I'm not sure I understand the underlying algorithm of the code anyway. To be clear the 'issue' here is related to the attempt to fix the ref-napi performance issue not the original ref-napi code which functionally works fine for me but the performance issue is such that for now I have to stick with the ref(Nan) packages. If anyone familiar with the ref-napi & ffi-napi code would like me to try testing any code changes to see if they help I'm willing to do so. |
|
Hi. I'll keep this update brief. I recognized that my attempted fix regarding using 'std::max<size_t>(length,1)' within LookupOrCreateArray was wrong even though it worked. A more appropriate fix which I believe is correct/expected behavior within ReadPointer is to add the following check after the reinterpret_cast to retrieve the value of the Pointer: if(val != nullPtr && size == 0) I've applied this fix & run the ref-napi tests & they all pass. Unfortunately ffi-napi fails immediately with a core dump when trying to run it's tests, specifically when first trying to run the ffi_prep_cif C function call from the libffi library (e.g. within the FFIPrepCif function call supplied by the ffi-napi library). Far be it from me to believe I actually totally understand the expectation of the ref-napi code but I do believe the above 'fix' is correct in the context of the fixes supplied to address ref-napi performance & the 'ERR_OUT_OF_BOUNDS' fix. |
…his will result in Buffers being allocated to the size requested up to kMaxLength (0x3fffffff) instead of the current behavior of always allocating kMaxLength. Since these buffers are allocated in the node external memory (limited to 64M) the old behavior would very quickly cause massive GC pressure as the GC is triggered on each new Buffer allocation once the node external memory limit is reached. It appears the old behavior was not intentional and should have always been std::min(). This should fix node-ffi-napi#72
…his will result in Buffers being allocated to the size requested up to kMaxLength (0x3fffffff) instead of the current behavior of always allocating kMaxLength. Since these buffers are allocated in the node external memory (limited to 64M) the old behavior would very quickly cause massive GC pressure as the GC is triggered on each new Buffer allocation once the node external memory limit is reached. It appears the old behavior was not intentional and should have always been std::min(). This should fix node-ffi-napi#72
Ever since Node.js 14 support got added to this library, and when run under Node.js 14+, we're having very slow performance when using this module.
Investigation showed that every external node::Buffer allocation would lead to a GC, and indeed, a lot of allocations make a buffer of
0x3fffffffin length leading to the external memory country in V8 getting angry.Looking a bit further, we find this bit of code:
ref-napi/src/binding.cc
Line 144 in a7f62a4
... which uses
std::maxto make every allocation the maximum length, which in this case is 0x3fffffff.Sadly, changing this to
std::minmakes for a lot of intermittent failures of theCheck failed: result.second.kind returning when run repeatedly. I guess the initial Node.js 14+ support never worked correctly and relied on this broken behavior.The text was updated successfully, but these errors were encountered: