net: improve performance of isIPv4 and isIPv6#49568
net: improve performance of isIPv4 and isIPv6#49568nodejs-github-bot merged 5 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
|
Hey @Uzlopak, can you make sure the commit message conforms to the commit message guideline? |
|
Just a remark: when i use following regex, i get even better perf. I think it reduces the backtracking. but i am not sure why. Do we have some Regex Expert?
node benchmark/is-ipv4.js |
|
how can i edit the commit message? I would probably just undo last commit and force push. But what should be the commit message to conform? |
|
You should amend and update your commit and force push using this guideline: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines |
|
Also modified the IPv6 regex. here is the benchmark: 'use strict'
const { isIPv6: isIPv6builtIn } = require('net')
const benchmark = require('benchmark')
const suite = new benchmark.Suite()
const ipLocal = '::1'
const isIPv6OrigRE = /^((?:(?:[0-9a-fA-F]{1,4}):){7}(?:(?:[0-9a-fA-F]{1,4})|:)|(?:(?:[0-9a-fA-F]{1,4}):){6}(?:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|:(?:[0-9a-fA-F]{1,4})|:)|(?:(?:[0-9a-fA-F]{1,4}):){5}(?::((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(:(?:[0-9a-fA-F]{1,4})){1,2}|:)|(?:(?:[0-9a-fA-F]{1,4}):){4}(?:(:(?:[0-9a-fA-F]{1,4})){0,1}:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(:(?:[0-9a-fA-F]{1,4})){1,3}|:)|(?:(?:[0-9a-fA-F]{1,4}):){3}(?:(:(?:[0-9a-fA-F]{1,4})){0,2}:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(:(?:[0-9a-fA-F]{1,4})){1,4}|:)|(?:(?:[0-9a-fA-F]{1,4}):){2}(?:(:(?:[0-9a-fA-F]{1,4})){0,3}:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(:(?:[0-9a-fA-F]{1,4})){1,5}|:)|(?:(?:[0-9a-fA-F]{1,4}):){1}(?:(:(?:[0-9a-fA-F]{1,4})){0,4}:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(:(?:[0-9a-fA-F]{1,4})){1,6}|:)|(?::((?::(?:[0-9a-fA-F]{1,4})){0,5}:((?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,7}|:)))(%[0-9a-zA-Z-.:]{1,})?$/
const isIPv6RE = /^(?:(?:(?:[0-9a-fA-F]{1,4}):){7}(?:(?:[0-9a-fA-F]{1,4})|:)|(?:(?:[0-9a-fA-F]{1,4}):){6}(?:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|:(?:[0-9a-fA-F]{1,4})|:)|(?:(?:[0-9a-fA-F]{1,4}):){5}(?::(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,2}|:)|(?:(?:[0-9a-fA-F]{1,4}):){4}(?:(?::(?:[0-9a-fA-F]{1,4})){0,1}:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,3}|:)|(?:(?:[0-9a-fA-F]{1,4}):){3}(?:(?::(?:[0-9a-fA-F]{1,4})){0,2}:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,4}|:)|(?:(?:[0-9a-fA-F]{1,4}):){2}(?:(?::(?:[0-9a-fA-F]{1,4})){0,3}:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,5}|:)|(?:(?:[0-9a-fA-F]{1,4}):){1}(?:(?::(?:[0-9a-fA-F]{1,4})){0,4}:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,6}|:)|(?::(?:(?::(?:[0-9a-fA-F]{1,4})){0,5}:(?:(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])[.]){3}(?:[0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])|(?::(?:[0-9a-fA-F]{1,4})){1,7}|:)))(?:%[0-9a-zA-Z-.:]{1,})?$/
suite.add('RE (modified)', function () {
isIPv6RE.test(ipLocal)
})
suite.add('RE (original)', function () {
isIPv6OrigRE.test(ipLocal)
})
suite.add('builtIn', function () {
isIPv6builtIn(ipLocal)
})
suite.on('cycle', function onCycle(event) {
console.log(String(event.target))
})
suite.run({ async: false })node benchmark/is-ipv6.js |
|
@anonrig |
|
@Uzlopak could youn please add your benchmark using Node.js infrastructure in https://github.com/nodejs/node/tree/main/benchmark/net? |
|
Something like this? 'use strict';
const common = require('../common.js');
const { isIPv4 } = require('net');
const minIPv4 = '0.0.0.0';
const maxIPv4 = '255.255.255.255';
const invalid = '0.0.0.0.0';
const bench = common.createBenchmark(main, {
n: [1e8],
});
function main({ n }) {
bench.start();
for (let i = 0; i < n; ++i) {
isIPv4(minIPv4);
isIPv4(maxIPv4);
isIPv4(invalid);
}
bench.end(n);
} |
|
You can run it as well locally to verify if it's working. https://github.com/nodejs/node/blob/main/doc/contributing/writing-and-running-benchmarks.md |
|
Does this need some kind of backporting? Or do we have to wait till release of node 22? :D |
|
Bench CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1381/ |
Commit Queue failed- Loading data for nodejs/node/pull/49568 ✔ Done loading data for nodejs/node/pull/49568 ----------------------------------- PR info ------------------------------------ Title net: improve performance of isIPv4 and isIPv6 (#49568) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch Uzlopak:patch-1 -> nodejs:main Labels net, performance, author ready, needs-ci Commits 5 - net: improve performance of isIPv4 and isIPv6 - Update lib/internal/net.js - add benchmarks - improve benchmarks as requested - add trailing comma Committers 2 - uzlopak - GitHub PR-URL: https://github.com/nodejs/node/pull/49568 Reviewed-By: Matteo Collina Reviewed-By: Yagiz Nizipli Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49568 Reviewed-By: Matteo Collina Reviewed-By: Yagiz Nizipli Reviewed-By: Paolo Insogna Reviewed-By: Luigi Pinca -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 08 Sep 2023 22:58:07 GMT ✔ Approvals: 4 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49568#pullrequestreview-1618630253 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49568#pullrequestreview-1618702262 ✔ - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/49568#pullrequestreview-1619385610 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/49568#pullrequestreview-1625294083 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-09-12T01:06:42Z: https://ci.nodejs.org/job/node-test-pull-request/53901/ ℹ Last Benchmark CI on 2023-09-12T07:17:37Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1381/ - Querying data for job/node-test-pull-request/1381/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 49568 From https://github.com/nodejs/node * branch refs/pull/49568/merge -> FETCH_HEAD ✔ Fetched commits as f5bb2c7d205c..d4675cf5801e -------------------------------------------------------------------------------- [main b0432b4569] net: improve performance of isIPv4 and isIPv6 Author: uzlopak Date: Sat Sep 9 01:42:49 2023 +0200 1 file changed, 10 insertions(+), 10 deletions(-) [main 6cd44bb65e] Update lib/internal/net.js Author: Uzlopak Date: Sat Sep 9 02:05:58 2023 +0200 1 file changed, 1 insertion(+), 1 deletion(-) [main 6dfa053218] add benchmarks Author: uzlopak Date: Sat Sep 9 15:30:49 2023 +0200 2 files changed, 44 insertions(+) create mode 100644 benchmark/net/net-is-ip-v4.js create mode 100644 benchmark/net/net-is-ip-v6.js [main 9b7d63212d] improve benchmarks as requested Author: uzlopak Date: Sun Sep 10 21:00:18 2023 +0200 2 files changed, 16 insertions(+), 12 deletions(-) [main c118049402] add trailing comma Author: uzlopak Date: Mon Sep 11 01:41:36 2023 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 5 commits in the PR. Attempting autorebase. Rebasing (2/10)https://github.com/nodejs/node/actions/runs/6177090293 |
|
Landed in 4e01842 |
PR-URL: #49568 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#49568 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Copy over the `isIPv4`/`isIPv6` performance improvements from <nodejs/node#49568>.
Copy over the `isIPv4`/`isIPv6` performance improvements from <nodejs/node#49568>.
Basically making the a capturing group to be non capturing.
I dont know how to bench the performance on nodejs
This is my benchmark:
result:
aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/proxy-addr$ node benchmark/is-ipv4.js
RE x 12,055,141 ops/sec ±1.49% (93 runs sampled)
builtIn x 8,672,332 ops/sec ±0.70% (94 runs sampled)