Skip to content

deps: V8: cherry-pick b49206d fom upstream#20727

Closed
ofrobots wants to merge 1 commit intonodejs:masterfrom
ofrobots:backport-v8-5338
Closed

deps: V8: cherry-pick b49206d fom upstream#20727
ofrobots wants to merge 1 commit intonodejs:masterfrom
ofrobots:backport-v8-5338

Conversation

@ofrobots
Copy link
Contributor

@ofrobots ofrobots commented May 14, 2018 •
edited
Loading

Ref: #20083

Original commit message:

ThreadDataTable: Change global linked list to per-Isolate hash map.

For use cases with a large number of threads or a large number of isolates (or
both), ThreadDataTable can be a major performance bottleneck due to O(n)
lookup time of the linked list. Switching to a hash map reduces this to O(1).

Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was observed
spending the majority of CPU time iterating over the ThreadDataTable.
See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

Example 2: Cloudflare's Workers engine, a high-multi-tenancy web server
framework built on V8 (but not Node), creates large numbers of threads and
isolates per-process. It saw a 34x improvement in throughput when we applied
this patch.

Cloudflare has been using a patch in production since the Workers launch which
replaces the linked list with a hash map -- but still global.

This commit builds on that but goes further and creates a separate hash map
and mutex for each isolate, with the table being a member of the Isolate
class. This avoids any globals and should reduce lock contention.

Bug: v8:5338
Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
Reviewed-on: https://chromium-review.googlesource.com/1014407
Reviewed-by: Yang Guo [email protected]
Commit-Queue: Yang Guo [email protected]
Cr-Commit-Position: refs/heads/master@{#52753}

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

CI: https://ci.nodejs.org/job/node-test-pull-request/14871/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1368/

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels May 14, 2018
@ofrobots ofrobots mentioned this pull request May 14, 2018
Closed
@ofrobots
deps: V8: cherry-pick b49206d from upstream
183d263 
Original commit message:
  ThreadDataTable: Change global linked list to per-Isolate hash map.

  For use cases with a large number of threads or a large number of isolates (or
  both), ThreadDataTable can be a major performance bottleneck due to O(n)
  lookup time of the linked list. Switching to a hash map reduces this to O(1).

  Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was observed
  spending the majority of CPU time iterating over the ThreadDataTable.
  See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

  Example 2: Cloudflare's Workers engine, a high-multi-tenancy web server
  framework built on V8 (but not Node), creates large numbers of threads and
  isolates per-process. It saw a 34x improvement in throughput when we applied
  this patch.

  Cloudflare has been using a patch in production since the Workers launch which
  replaces the linked list with a hash map -- but still global.

  This commit builds on that but goes further and creates a separate hash map
  and mutex for each isolate, with the table being a member of the Isolate
  class. This avoids any globals and should reduce lock contention.

  Bug: v8:5338
  Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
  Reviewed-on: https://chromium-review.googlesource.com/1014407
  Reviewed-by: Yang Guo <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#52753}

PR-URL: nodejs#20727
Ref: nodejs#20083
@ofrobots ofrobots force-pushed the backport-v8-5338 branch from a46a732 to 183d263 Compare May 14, 2018 20:18
bnoordhuis
bnoordhuis approved these changes May 15, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. New CI because the previous one bombed on infrastructure issues:

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented May 17, 2018

New CI was all green: https://ci.nodejs.org/job/node-test-commit/18544/. Landing.

@ofrobots
Copy link
Contributor Author

ofrobots commented May 17, 2018

Landed in 0ebbd76.

@ofrobots ofrobots closed this May 17, 2018
@ofrobots ofrobots deleted the backport-v8-5338 branch May 17, 2018 18:25
ofrobots added a commit that referenced this pull request May 17, 2018
@ofrobots
deps: V8: cherry-pick b49206d from upstream
0ebbd76 
Original commit message:
  ThreadDataTable: Change global linked list to per-Isolate hash map.

  For use cases with a large number of threads or a large number of
  isolates (or both), ThreadDataTable can be a major performance
  bottleneck due to O(n) lookup time of the linked list. Switching to a
  hash map reduces this to O(1).

  Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was
  observed spending the majority of CPU time iterating over the
  ThreadDataTable.
  See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

  Example 2: Cloudflare's Workers engine, a high-multi-tenancy web
  server framework built on V8 (but not Node), creates large numbers of
  threads and isolates per-process. It saw a 34x improvement in
  throughput when we applied this patch.

  Cloudflare has been using a patch in production since the Worker
  launch which replaces the linked list with a hash map -- but still
  global.

  This commit builds on that but goes further and creates a separate
  hash map and mutex for each isolate, with the table being a member of
  the Isolate class. This avoids any globals and should reduce lock
  contention.

  Bug: v8:5338
  Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
  Reviewed-on: https://chromium-review.googlesource.com/1014407
  Reviewed-by: Yang Guo <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#52753}

PR-URL: #20727
Ref: #20083

Refs: #20083
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@KoenLav
Copy link

KoenLav commented May 21, 2018

@ofrobots is there anything still needed to get this into Node.js 8?

@ofrobots
Copy link
Contributor Author

ofrobots commented May 21, 2018

@KoenLav the process we follow is that a fix first has to be available on the current release line before it is considered for back-porting to LTS by the @nodejs/release team. I have attached the labels above which will make this fix show up on the list of potential back-ports to the LTS releases.

It is going to take a little while before you see it on 8 LTS – but the objective is to make sure we have adequate feedback from the current line to avoid disrupting the LTS lines.. /cc @nodejs/release in case I got anything wrong about the release process.

MylesBorins pushed a commit that referenced this pull request May 22, 2018
@ofrobots @MylesBorins
deps: V8: cherry-pick b49206d from upstream
91e60b0 
Original commit message:
  ThreadDataTable: Change global linked list to per-Isolate hash map.

  For use cases with a large number of threads or a large number of
  isolates (or both), ThreadDataTable can be a major performance
  bottleneck due to O(n) lookup time of the linked list. Switching to a
  hash map reduces this to O(1).

  Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was
  observed spending the majority of CPU time iterating over the
  ThreadDataTable.
  See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

  Example 2: Cloudflare's Workers engine, a high-multi-tenancy web
  server framework built on V8 (but not Node), creates large numbers of
  threads and isolates per-process. It saw a 34x improvement in
  throughput when we applied this patch.

  Cloudflare has been using a patch in production since the Worker
  launch which replaces the linked list with a hash map -- but still
  global.

  This commit builds on that but goes further and creates a separate
  hash map and mutex for each isolate, with the table being a member of
  the Isolate class. This avoids any globals and should reduce lock
  contention.

  Bug: v8:5338
  Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
  Reviewed-on: https://chromium-review.googlesource.com/1014407
  Reviewed-by: Yang Guo <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#52753}

PR-URL: #20727
Ref: #20083

Refs: #20083
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request May 22, 2018
Merged
@veered
Copy link

veered commented Jun 22, 2018

@ofrobots Any idea on when this might land in Node 8? It'll be a huge deal for the Meteor community :)

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 22, 2018

@MylesBorins is this on track for a 8.x release?

@MylesBorins
Copy link
Contributor

MylesBorins commented Jun 25, 2018

@ofrobots this is going to need a manual backport to 8.x

ping @nodejs/v8-update

@ofrobots ofrobots mentioned this pull request Jun 25, 2018
Merged
2 tasks
@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 25, 2018

8.x backport: #21529

ofrobots added a commit to ofrobots/node that referenced this pull request Jun 28, 2018
@ofrobots
deps: V8: backport b49206d from upstream
646445b 
This is the v8.x backport of nodejs#20727.

Original commit message:
  ThreadDataTable: Change global linked list to per-Isolate hash map.

  For use cases with a large number of threads or a large number of
  isolates (or both), ThreadDataTable can be a major performance
  bottleneck due to O(n) lookup time of the linked list. Switching to a
  hash map reduces this to O(1).

  Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was
  observed spending the majority of CPU time iterating over the
  ThreadDataTable.
  See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

  Example 2: Cloudflare's Workers engine, a high-multi-tenancy web
  server framework built on V8 (but not Node), creates large numbers of
  threads and isolates per-process. It saw a 34x improvement in
  throughput when we applied this patch.

  Cloudflare has been using a patch in production since the Worker
  launch which replaces the linked list with a hash map -- but still
  global.

  This commit builds on that but goes further and creates a separate
  hash map and mutex for each isolate, with the table being a member of
  the Isolate class. This avoids any globals and should reduce lock
  contention.

  Bug: v8:5338
  Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
  Reviewed-on: https://chromium-review.googlesource.com/1014407
  Reviewed-by: Yang Guo <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#52753}

Backport-PR-URL: nodejs#21529
PR-URL: nodejs#20727
Refs: nodejs#20083
Refs: nodejs#20083
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
Merged
@veered veered mentioned this pull request Jul 20, 2018
Closed
@jimrandomh jimrandomh mentioned this pull request Jul 31, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@ofrobots @rvagg
deps: V8: backport b49206d from upstream
f987a51 
This is the v8.x backport of #20727.

Original commit message:
  ThreadDataTable: Change global linked list to per-Isolate hash map.

  For use cases with a large number of threads or a large number of
  isolates (or both), ThreadDataTable can be a major performance
  bottleneck due to O(n) lookup time of the linked list. Switching to a
  hash map reduces this to O(1).

  Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was
  observed spending the majority of CPU time iterating over the
  ThreadDataTable.
  See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story

  Example 2: Cloudflare's Workers engine, a high-multi-tenancy web
  server framework built on V8 (but not Node), creates large numbers of
  threads and isolates per-process. It saw a 34x improvement in
  throughput when we applied this patch.

  Cloudflare has been using a patch in production since the Worker
  launch which replaces the linked list with a hash map -- but still
  global.

  This commit builds on that but goes further and creates a separate
  hash map and mutex for each isolate, with the table being a member of
  the Isolate class. This avoids any globals and should reduce lock
  contention.

  Bug: v8:5338
  Change-Id: If0d11509afb2e043b888c376e36d3463db931b47
  Reviewed-on: https://chromium-review.googlesource.com/1014407
  Reviewed-by: Yang Guo <[email protected]>
  Commit-Queue: Yang Guo <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#52753}

Backport-PR-URL: #21529
PR-URL: #20727
Refs: #20083
Refs: #20083
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ofrobots @KoenLav @veered @MylesBorins @bnoordhuis @jasnell @richardlau @BridgeAR @nodejs-github-bot