-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
benchmarks: fix benchmark for url #19084
Conversation
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.
It would be great to add a test for the url benchmarks to prevent any further regressions. Would you be so kind and add one?
A alternative way of fixing this would be replacing the fn
in the for loop with params[method](param);
. I personally would go for that instead.
@BridgeAR I replaced |
@BridgeAR tell me pls, how can I add a test for a benchmark? |
Done |
test/parallel/test-benchmark-url.js
Outdated
|
||
const runBenchmark = require('../common/benchmark'); | ||
|
||
runBenchmark('url', ['n=1'], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); |
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.
Please also add options for all existing options in any url benchmark that exists and pass in a concrete value. Otherwise it would test for all options and that increases the runtime significantly.
So e.g. method
, type
...
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.
@BridgeAR when I add option like 'method=get' or 'method=' it throws an error:
/Users/daynin/Documents/projects/node/benchmark/url/legacy-vs-whatwg-url-get-prop.js:89
throw new Error(`Unknown method "${method}"`);
^
Error: Unknown method "get"
or
/Users/daynin/Documents/projects/node/benchmark/url/legacy-vs-whatwg-url-get-prop.js:89
throw new Error(`Unknown method "${method}"`);
^
Error: Unknown method ""
Can I specify certain benchmark for the test somehow or avoid these errors?
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.
Seems like you're passing in method="get"
given the quotation marks in the error? Instead pass in method=get
.
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.
@apapirovski I passed 'method=get' as an option. But I think the problem is url-searchparams-read.js
has than config:
const bench = common.createBenchmark(main, {
method: ['get', 'getAll', 'has'],
param: ['one', 'two', 'three', 'nonexistent'],
n: [2e7]
})
but legacy-vs-whatwg-url-get-prop.js
has that config:
const bench = common.createBenchmark(main, {
type: Object.keys(inputs),
method: ['legacy', 'whatwg'],
n: [1e5]
})
There are two params method
with different values
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.
Ah, sorry, didn't see the file name you mentioned. I would probably just rename the method
to be something else in the legacy benchmark, if that's the only one blocking this.
@apapirovski @BridgeAR done |
fbb327b
to
98d406f
Compare
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.
Not a fan of the churn for this change but the code itself looks good to me.
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 but needs a typo fix-up before landing.
test/parallel/test-benchmark-url.js
Outdated
'accessMethod=get', | ||
'type=short', | ||
'searchParam=noencode', | ||
'herf=short', |
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.
Typo? Should be href
, I think.
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.
@apapirovski yeah, of course
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.
@apapirovski fixed
- rename different parameters with the same names to make it possible to run tests for url benchmarks - add options in the test for all url benchmarks
/cc @nodejs/collaborators Hello! Run CI for this PR, please |
Landed in f3257dd 🎉 |
Rename different parameters with the same names to make it possible to run tests for url benchmarks. PR-URL: nodejs#19084 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>
Rename different parameters with the same names to make it possible to run tests for url benchmarks. PR-URL: #19084 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>
Rename different parameters with the same names to make it possible to run tests for url benchmarks. PR-URL: #19084 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>
Rename different parameters with the same names to make it possible to run tests for url benchmarks. PR-URL: nodejs#19084 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Shingo Inoue <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>
Should this be backported to 8.x? If so, a separate backport PR is needed |
It fixes an error which occurs after running this test:
Error:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
benchmark