★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25410Nouvelle recherche | Portail wanayoo
Skip to content

/node

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.

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

os: improve networkInterfaces() performance #25410

Closed
wants to merge 1 commit into from

Conversation

@mscdex
Copy link
Contributor

commented Jan 9, 2019 •
edited

With included benchmark:

                                  confidence improvement accuracy (*)   (**)  (***)
 os/networkInterfaces.js n=10000        ***      3.86 %       ±0.26% ±0.34% ±0.44%

and also makes the C++ implementation mirror the C++ cpu enumeration code for whatever that's worth.

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mscdex mscdex added os performance labels Jan 9, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jan 9, 2019

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

@nodejs-github-bot nodejs-github-bot added the C++ label Jan 9, 2019

@jdalton
jdalton reviewed Jan 9, 2019
View changes
lib/os.js
if (existing !== undefined)
existing.push(entry);
else
result[name] = [entry];
}

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Jan 9, 2019 •
edited

Member

Can you explain the perf benefit for networkInterfaces()?

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Jan 9, 2019

Author Contributor

What do you mean?

This comment has been minimized.

Sign in to view
Copy link
@jdalton

jdalton Jan 9, 2019 •
edited

Member

At first glance it looks like a lot of unrolled code with an extra object being created. Could you please step me through the optimization being done in networkInterfaces(). If you drop the rework to networkInterfaces() do you still see perf wins?

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Jan 9, 2019 •
edited

Author Contributor

It's more or less moving the object creation from C++ to JS land, just like we do for CPU enumeration.
@BridgeAR
BridgeAR reviewed Jan 9, 2019
View changes
Copy link
Member

left a comment

I personally wonder if it would not be faster to just move everything to C++ instead.

The changes LGTM besides the result object. It should be kept as regular object.
lib/os.js Outdated Show resolved Hide resolved
@jasnell
jasnell approved these changes Jan 9, 2019
View changes
@mscdex
os: improve networkInterfaces() performance
Verified
This commit was signed with a verified signature.
mscdex mscdex
GPG key ID: 606D7358F94DA209 Learn about signing commits
Loading status checks…
b505976

@mscdex mscdex force-pushed the mscdex:os-networkinterfaces-refactor branch from d7b7cde to b505976 Jan 9, 2019

@BridgeAR
BridgeAR approved these changes Jan 10, 2019
View changes
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 10, 2019

Resumed CI https://ci.nodejs.org/job/node-test-commit/24935/

@BridgeAR BridgeAR added the author ready label Jan 10, 2019

@danbev

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 14, 2019

Landed in 5021b25.

@danbev danbev closed this Jan 14, 2019

danbev added a commit that referenced this pull request Jan 14, 2019
@mscdex @danbev
os: improve networkInterfaces() performance 
PR-URL: #25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Loading status checks…
5021b25

@mscdex mscdex deleted the mscdex:os-networkinterfaces-refactor branch Jan 14, 2019

addaleax added a commit that referenced this pull request Jan 14, 2019
@mscdex @addaleax
os: improve networkInterfaces() performance 
PR-URL: #25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
840ec23
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
Merged
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@mscdex @BridgeAR
os: improve networkInterfaces() performance 
PR-URL: nodejs#25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
173f9d0
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdalton jdalton

@jasnell jasnell

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
C++ author ready os performance
Projects
None yet
Milestone
No milestone
6 participants
@mscdex @nodejs-github-bot @BridgeAR @danbev @jdalton @jasnell
You can’t perform that action at this time.