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.

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

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
if (existing !== undefined)
existing.push(entry);
else
result[name] = [entry];
}

This comment has been minimized.

Copy link
@jdalton

jdalton Jan 9, 2019

Member

Can you explain the perf benefit for networkInterfaces()?

This comment has been minimized.

Copy link
@mscdex

mscdex Jan 9, 2019

Author Contributor

What do you mean?

This comment has been minimized.

Copy link
@jdalton

jdalton Jan 9, 2019

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.

Copy link
@mscdex

mscdex Jan 9, 2019

Author Contributor

It's more or less moving the object creation from C++ to JS land, just like we do for CPU enumeration.

@BridgeAR
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

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

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

@danbev

This comment has been minimized.

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
os: improve networkInterfaces() performance
PR-URL: #25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

addaleax added a commit that referenced this pull request Jan 14, 2019
os: improve networkInterfaces() performance
PR-URL: #25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
os: improve networkInterfaces() performance
PR-URL: nodejs#25410
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.