os: improve networkInterfaces performance #22359

BridgeAR · 2018-08-16T12:38:41Z

This algorithm uses less data transformations and is therefore
significantly faster than the one before.

The main pain point is how the CIDR suffix is calculated. It would
be best to port this to C++ instead of having to do this in JS as all other
transformations are done in C++. But since this improves the
performance already quite a bit it's a good first step.

Performance:

> console.time(); for(var i = 0; i < 10000; i++) os.networkInterfaces(); console.timeEnd()

// Before
default: 1170.912ms

// After:
default: 758.899ms

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-16T12:38:42Z

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

BridgeAR · 2018-08-16T13:33:45Z

~~CI https://ci.nodejs.org/job/node-test-pull-request/16485/~~
CI https://ci.nodejs.org/job/node-test-pull-request/16487/

mafintosh · 2018-08-16T13:44:52Z

LGTM, nice!

BridgeAR · 2018-08-16T22:17:10Z

I just pushed another commit that optimizes the algorithm. It is now a much simpler and straight forward as I calculate the number of ones right away and use modulo 2 to validate the entry.

New benchmark result:

> console.time(); for(var i = 0; i < 10000; i++) os.networkInterfaces(); console.timeEnd()
default: 758.899ms

lib/os.js

BridgeAR · 2018-08-16T23:44:54Z

~~New CI https://ci.nodejs.org/job/node-test-pull-request/16506/~~
New CI https://ci.nodejs.org/job/node-test-pull-request/16512/

lpinca · 2018-08-18T19:09:35Z

lib/os.js

+  }
+
+  const parts = netmask.split(split);
+  for (const part of parts) {


Since you are rewriting this for performance, is for...of as fast as the classic for loop now?

Tracking bug: https://bugs.chromium.org/p/v8/issues/detail?id=8070

lpinca · 2018-08-18T19:12:49Z

lib/os.js

+          return null;
+        }
+      } else if (tmp !== groupLength) {
+        if (binary % 2 !== 0) {


You could use (binary & 1) !== 0. Last time I tried the reminder operator was not optimized for this, maybe it is now, don't know.

Tracking bug https://bugs.chromium.org/p/v8/issues/detail?id=8069

Fixed in 6.0: https://crrev.com/35974e2da664fdbc5678b540e2ab0c64b1bbd182

lib/os.js

BridgeAR · 2018-08-19T15:36:41Z

Comments addressed. The performance is now again a tiny bit improved compared to before by using:

Simple for loops
No Object.values
Using the binary operator instead of modulo

BridgeAR · 2018-08-19T15:37:31Z

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

This algorithm uses less data transformations and is therefore significantly faster than the one before.

BridgeAR · 2018-08-19T17:00:24Z

Rebased due to a hiccup with the CI. I only squashed commits that were already reviewed.

CI https://ci.nodejs.org/job/node-test-pull-request/16576/ ✔️

mafintosh · 2018-08-19T17:13:16Z

Still LGTM

BridgeAR · 2018-08-20T12:38:20Z

@addaleax @lpinca mind taking another look?

This algorithm uses less data transformations and is therefore significantly faster than the one before. PR-URL: nodejs#22359 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

BridgeAR · 2018-08-20T14:11:55Z

Landed in 9bcb744 🎉

This algorithm uses less data transformations and is therefore significantly faster than the one before. PR-URL: #22359 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. os Issues and PRs related to the os subsystem. labels Aug 16, 2018

BridgeAR added the performance Issues and PRs related to the performance of Node.js. label Aug 16, 2018

BridgeAR requested a review from mafintosh August 16, 2018 12:39

BridgeAR force-pushed the improve-comparison-performance branch 2 times, most recently from e7c90b9 to bb4a589 Compare August 16, 2018 13:30

BridgeAR force-pushed the improve-comparison-performance branch from bb4a589 to e625c9d Compare August 16, 2018 13:38

mafintosh approved these changes Aug 16, 2018

View reviewed changes

jasnell approved these changes Aug 16, 2018

View reviewed changes

TimothyGu approved these changes Aug 16, 2018

View reviewed changes

maclover7 reviewed Aug 16, 2018

View reviewed changes

lib/os.js Outdated Show resolved Hide resolved

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 16, 2018

This was referenced Aug 17, 2018

Git lock in test-azure_msft-win10-x64-3 exists nodejs/build#1460

Closed

CI Failures: 20180817 nodejs/reliability#14

Closed

lpinca reviewed Aug 18, 2018

View reviewed changes

addaleax reviewed Aug 19, 2018

View reviewed changes

lib/os.js Outdated Show resolved Hide resolved

BridgeAR added 2 commits August 19, 2018 18:56

os: improve networkInterfaces performance

f7c1aed

This algorithm uses less data transformations and is therefore significantly faster than the one before.

fixup: address comments

17f4930

BridgeAR force-pushed the improve-comparison-performance branch from 0baa2d7 to 17f4930 Compare August 19, 2018 16:57

joyeecheung mentioned this pull request Aug 20, 2018

test-digitalocean-fedora27-x64-1 keeps timing out nodejs/build#1463

Closed

joyeecheung mentioned this pull request Aug 20, 2018

CI failures: 20180820 nodejs/reliability#15

Closed

lpinca approved these changes Aug 20, 2018

View reviewed changes

BridgeAR closed this Aug 20, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BridgeAR mentioned this pull request Dec 20, 2019

for ... of replacing for(;;) in library networking code? #31024

Closed

BridgeAR deleted the improve-comparison-performance branch January 20, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: improve networkInterfaces performance #22359

os: improve networkInterfaces performance #22359

BridgeAR commented Aug 16, 2018 •

edited

Loading

nodejs-github-bot commented Aug 16, 2018

BridgeAR commented Aug 16, 2018 •

edited

Loading

mafintosh commented Aug 16, 2018

BridgeAR commented Aug 16, 2018 •

edited

Loading

BridgeAR commented Aug 16, 2018 •

edited

Loading

lpinca Aug 18, 2018

BridgeAR Aug 19, 2018

lpinca Aug 18, 2018

BridgeAR Aug 19, 2018

BridgeAR Aug 20, 2018

BridgeAR commented Aug 19, 2018

BridgeAR commented Aug 19, 2018

BridgeAR commented Aug 19, 2018 •

edited

Loading

mafintosh commented Aug 19, 2018

BridgeAR commented Aug 20, 2018

BridgeAR commented Aug 20, 2018

os: improve networkInterfaces performance #22359

os: improve networkInterfaces performance #22359

Conversation

BridgeAR commented Aug 16, 2018 • edited Loading

Checklist

nodejs-github-bot commented Aug 16, 2018

BridgeAR commented Aug 16, 2018 • edited Loading

mafintosh commented Aug 16, 2018

BridgeAR commented Aug 16, 2018 • edited Loading

BridgeAR commented Aug 16, 2018 • edited Loading

lpinca Aug 18, 2018

Choose a reason for hiding this comment

BridgeAR Aug 19, 2018

Choose a reason for hiding this comment

lpinca Aug 18, 2018

Choose a reason for hiding this comment

BridgeAR Aug 19, 2018

Choose a reason for hiding this comment

BridgeAR Aug 20, 2018

Choose a reason for hiding this comment

BridgeAR commented Aug 19, 2018

BridgeAR commented Aug 19, 2018

BridgeAR commented Aug 19, 2018 • edited Loading

mafintosh commented Aug 19, 2018

BridgeAR commented Aug 20, 2018

BridgeAR commented Aug 20, 2018

BridgeAR commented Aug 16, 2018 •

edited

Loading

BridgeAR commented Aug 16, 2018 •

edited

Loading

BridgeAR commented Aug 16, 2018 •

edited

Loading

BridgeAR commented Aug 16, 2018 •

edited

Loading

BridgeAR commented Aug 19, 2018 •

edited

Loading