Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cluster: use Maps to track handles, indexes, etc. #23125

Merged
merged 5 commits into from
Sep 30, 2018
Merged

cluster: use Maps to track handles, indexes, etc. #23125

merged 5 commits into from
Sep 30, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 27, 2018

Using Maps over POJOs is more idiomatic, and avoids many expensive delete operations. This PR only touches internal APIs.

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1041/pipeline

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Sep 27, 2018
@mscdex
Copy link
Contributor

mscdex commented Sep 27, 2018

Do we have benchmarks to compare a possible change in performance with these changes?

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 27, 2018 •
edited
Loading

There is only one cluster benchmark, but it seems to improve with these changes:

before:
cluster/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1: 20,023.718912019016
cluster/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1: 7,540.0656667259545
cluster/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1: 15,415.718044848363
cluster/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1: 5,995.307144914562

after:
cluster/echo.js n=100000 sendsPerBroadcast=1 payload="string" workers=1: 21,580.722871510236
cluster/echo.js n=100000 sendsPerBroadcast=10 payload="string" workers=1: 8,586.962318302709
cluster/echo.js n=100000 sendsPerBroadcast=1 payload="object" workers=1: 15,989.847697757608
cluster/echo.js n=100000 sendsPerBroadcast=10 payload="object" workers=1: 6,376.197549671379```

@mscdex
Copy link
Contributor

mscdex commented Sep 27, 2018 •
edited
Loading

The existing benchmark doesn't exercise the code paths being changed in this PR, since it is just passing messages and not handles. We need an additional benchmark that passes handles to the forked processes.

As far as the existing benchmark results go, I would expect the difference to not be statistically significant over many runs.

devsnek
devsnek approved these changes Sep 27, 2018
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

dunno about the speed, but the semantics look good 👍

targos
targos approved these changes Sep 27, 2018
View reviewed changes
lib/internal/cluster/utils.js
@@ -4,10 +4,10 @@ const util = require('util');
module.exports = {
sendHelper,
internal,
handles: {} // Used in tests.
handles: new Map() // Used in tests.
Copy link
Member

Choose a reason for hiding this comment

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

If this is used in tests, shouldn't tests be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@targos the only test that was using it was removed. See #23131

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 27, 2018

It might not be possible to create a benchmark that passes handles in a similar fashion. At least on macOS, Node hangs after ~10-11k "echoes" (on master even without this PR, more investigation is needed there) Additionally, on master, it seems that using a Map is a good bit faster (although I wouldn't expect the Map vs. Object decision to be the bottleneck in these IPC heavy applications anyway):

$ ./node benchmark/es/map-bench.js 
es/map-bench.js n=1000000 method="object": 355,619.17415044963
es/map-bench.js n=1000000 method="nullProtoObject": 369,429.81874460133
es/map-bench.js n=1000000 method="nullProtoLiteralObject": 365,293.87356241985
es/map-bench.js n=1000000 method="storageObject": 365,471.84898171073
es/map-bench.js n=1000000 method="fakeMap": 330,257.280692661
es/map-bench.js n=1000000 method="map": 642,936.8119129504

@cjihrig
cluster: use Map to track callbacks
97dad7e 
Use a Map to avoid delete operations in callback tracking.

PR-URL: nodejs#23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@cjihrig
cluster: use Map to track round robin workers
6654c59 
PR-URL: nodejs#23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@cjihrig
cluster: use Map to track indexes
aa899aa 
PR-URL: nodejs#23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@cjihrig
cluster: use Map to track handles in cluster child
5a50989 
PR-URL: nodejs#23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@cjihrig
cluster: use Map to track handles in master
847037e 
PR-URL: nodejs#23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 30, 2018

Green CI: https://ci.nodejs.org/job/node-test-pull-request/17514/

@cjihrig cjihrig merged commit 847037e into nodejs:master Sep 30, 2018
@cjihrig cjihrig deleted the cluster branch September 30, 2018 15:06
targos pushed a commit that referenced this pull request Oct 1, 2018
@cjihrig @targos
cluster: use Map to track callbacks
1172bc4 
Use a Map to avoid delete operations in callback tracking.

PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
@cjihrig @targos
cluster: use Map to track round robin workers
fc61175 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
@cjihrig @targos
cluster: use Map to track indexes
bdb4309 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
@cjihrig @targos
cluster: use Map to track handles in cluster child
c7a9360 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 1, 2018
@cjihrig @targos
cluster: use Map to track handles in master
278dbf2 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@cjihrig @targos
cluster: use Map to track callbacks
22f51a6 
Use a Map to avoid delete operations in callback tracking.

PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@cjihrig @targos
cluster: use Map to track round robin workers
64f840a 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@cjihrig @targos
cluster: use Map to track indexes
2dd157f 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@cjihrig @targos
cluster: use Map to track handles in cluster child
0f133eb 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
@cjihrig @targos
cluster: use Map to track handles in master
cb0d823 
PR-URL: #23125
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@devsnek devsnek devsnek approved these changes

@lundibundi lundibundi lundibundi approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @nodejs-github-bot @mscdex @jasnell @addaleax @targos @devsnek @lundibundi @trivikr