Repository navigation
100% CPU caused by thread spike (in Fibers, Meteor) #20083
Description
Version: 8.11.1
Platform: any
We have been experiencing 100% CPU in production, presumably caused by the use of Fibers in Node.js by Meteor and the way the data lookup for threads is implemented in V8.
While this issue probably should not be fixed in this repository it seemed worthwhile to track it here as well, in order to find out whether more users of Node.js are suffering from this issue.
As @kentonv describes the issue in the Meteor repository:
V8 uses a linked list to map thread IDs to some thread-local data. Fibers aren't threads, but the fibers package fakes out V8 into thinking that fibers are threads, so this applies to fibers too. If a large number of fibers are created, the list becomes long, and so every lookup into the table becomes slow, because a lookup in a linked list is O(n). These lookups happen frequently, slowing down everything. Using a hash table makes the lookups O(1).
Issue on Meteor repository:
meteor/meteor#9796
Issue on Fibers repository:
laverdet/node-fibers#371
Commit which fixes the issue in Node.js:
0b88256
V8 issue:
https://bugs.chromium.org/p/v8/issues/detail?id=5338
Proposed contribution which fixes the issue in V8:
Not available yet.
Activity
devsnek commentedon Apr 16, 2018
node already has the patch, and v8 is oss, why not just submit the patch to them? this also seems like the wrong place to be asking.
KoenLav commentedon Apr 16, 2018
@devsnek
"While this issue probably should not be fixed in this repository it seemed worthwhile to track it here as well, in order to find out whether more users of Node.js are suffering from this issue."
I've created a PR in the V8 repo: v8/v8#24
devsnek commentedon Apr 16, 2018
@KoenLav please note v8 doesn't accept prs to that repo, they use gerrit. you can follow this guide to learn how to submit a patch: http://dev.chromium.org/developers/contributing-code
i'm going to close this for now, feel free to ping me if you have any other questions.
apapirovski commentedon Apr 16, 2018
ping @nodejs/v8 just in case. Although seems like this is on the right track as is.
KoenLav commentedon Apr 16, 2018
@devsnek no problem to leave this here closed (people should still be able to find it more easily).
KoenLav commentedon Apr 24, 2018
@devsnek the V8 patch has landed (https://chromium-review.googlesource.com/c/v8/v8/+/1014407). Any indication of when it will be available in Node.js?
ofrobots commentedon Apr 24, 2018
@KoenLav I have requested an upstream merge for V8 6.7 and 6.6 so that that can end up in Node 10. Let's see if upstream is willing to back merge. Otherwise I'm +1 on floating it as a patch here otherwise.
devsnek commentedon Apr 24, 2018
@KoenLav you can run
git node v8 backport b49206ded97c4eaac7c273ce004d840a0185d40e(using https://github.com/nodejs/node-core-utils/) in a checkout of node and then open a profrobots commentedon Apr 24, 2018
Yes, but wait for a disposition upstream first.
KoenLav commentedon Apr 26, 2018
@devsnek @ofrobots awaiting merge from upstream first.
Any chance of getting this applied in Nodejs 8.* as well?
ofrobots commentedon Apr 26, 2018
Yes, that is possible. The LTS policy is that a fix has to be released on the current branch for a bit before we back-port to LTS branches. This helps shake any potential instability. The process as I see it, from here:
master. The release team will back-port and release on 10.If all looks good, this will subsequently be considered for a merge back to older release branches.
KoenLav commentedon May 3, 2018
@ofrobots as the merge has been approved for V8 6.7, what would be the proper course of action from here?
hashseed commentedon May 3, 2018
Our fuzzers found a crash so I would like to understand what's going on there before moving forward with this. @ofrobots has been CC'ed on the issue.
5 remaining items
ofrobots commentedon May 14, 2018
I've opened the 6.7 merge change upstream. Once there is LGTM, this can land upstream, and will be picked up by Node.js when we move up to V8 6.7 (~ end of the month).
For V8 6.6, we need to float the patch here. I've opened PR here: #20727
deps: V8: cherry-pick b49206d from upstream
deps: V8: cherry-pick b49206d from upstream
deps: V8: cherry-pick b49206d from upstream
deps: V8: backport b49206d from upstream