test: call gc() explicitly to avoid OOM #22301
Conversation
refack
added
flaky-test
nodejs-github-bot
added
addons
| @@ -35,10 +36,12 @@ common.expectsError(function() { | |||
| type: Error | |||
| }); | |||
|
|
|||
| // Free the memory early to avoid OOM | |||
| global.gc() | |||
There was a problem hiding this comment.
Maybe add a FIXME here since it's not supposed to crash? This is just a quick fix
refack
force-pushed
the
less-OOM-for-freebsd-by-max-string
branch
from
ca53596
to
d970a8a
Compare
|
/ping @nodejs/v8 on "shouldn't setting to |
| maxString = undefined; | ||
| global.gc() |
|
I've started another stress test (1989 pending the current one 1988) with master for comparison |
|
/CC @nodejs/testing some low hanging time optimization fruit: |
v8 has a fairly lazy gc by default. setting a variable to undefined clears the reference to the value but the value might not be deleted until the next gc, and even then it might be removed from v8's heap but v8's heap might not shrink. there is a new method in v8 called
v8 will allocate the second and third strings without deleting the previous ones. as far as i know v8 doesn't track how much memory the system actually has. because of this it has a method called someone actually brought up on irc yesterday this topic about having more control over how node (and by extension v8) is using memory. i'm wondering if maybe we could work on exposing some options around that to applications. |
|
@devsnek |
|
By the way I can't seem to reproduce this in a new FreeBSD box, maybe it has something to do with the resource usage on our CI machines and you'd need to hit a certain threshold to get the crash. @refack have you managed to get a core dump? |
|
@joyeecheung i meant that if v8 tries to allocate and hits OOM it will abort. there might also be a callback to catch that. i don't know what it does if it fails to deallocate. |
|
@devsnek The majority of the allocation is not supposed to happen in the V8 heap, since it's an external string...also nodejs/reliability#12 (comment) didn't give us the usual OOM message from V8, it's killed by the system so it's probably caused by some system call hitting the resource limit (whereas if the OOM happens in V8's heap V8 will print that OOM message before aborting). The actual V8 heap is only a few megabytes in https://www.irccloud.com/pastebin/FI3zgCwR/ |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/16431/ |
Stress test against But I think that is a significant enough result. |
refack
added
the
author ready
|
👍 here to fast-track. |
|
Resume build: https://ci.nodejs.org/job/node-test-pull-request/16433/ |
|
Although it looks like https://ci.nodejs.org/job/node-test-pull-request/16432/ is a Resume Build that's already green so mine is unnecessary. This can land once we get a second fast-track approval. Someone? Anyone? |
@devsnek - does this mean that (large and complex) applications should explicitly invoke gc() to avoid garbage pileup? that is in direct contradiction with the automatic garbage collection promise. I would assume gc() to abort only after it has cleared up even the last byte of garbage and still cannot allocate the current request. To me, this looks like a v8 bug. |
|
@ulan @hannespayer I'm also a bit surprised that you need to explicitly call GC to avoid OOM. |
that is how v8 behaves (https://github.com/v8/v8/blob/3f0346cdedb230a89dc433ade7e46ac7129ba5e4/src/heap/heap.cc#L4608-L4648) there was some missing context here... this test allocates external strings, not v8 heap strings, so from what i understand the failure is actually a bug with node, not v8. this fact, of course, plugs into hashseed's comment. |
PR-URL: nodejs#22301 Refs: nodejs/reliability#12 Refs: nodejs#16354 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
refack
force-pushed
the
less-OOM-for-freebsd-by-max-string
branch
from
4757c91
to
4ce744a
Compare
|
Landed in 4ce744a |
|
EDIT: that CI was not rebased against this commit |
|
tl;dr: This should be fixed by V8 soon. V8 relies on embedders calling AdjustAmountOfExternalAllocatedMemory for memory that externally allocated. This is error prone and the API does not follow any growing strategy for allocated memory, making it likely that it is not doing not enough or too many GCs. We know that this is an issue and there's ongoing work that is about to be landed on master handling types where V8 has more knowledge (externalized strings & array buffers). Specifically, the external string issue should be fixed as soon as that CL lands. |
PR-URL: #22301 Refs: nodejs/reliability#12 Refs: #16354 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
PR-URL: #22301 Refs: nodejs/reliability#12 Refs: #16354 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
Refs: nodejs/reliability#12
Refs: #16354
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes