-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
net: improve performance of isIPv4 and isIPv6 #49568
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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>.
…#8271) 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)