benchmarks: fix benchmark for url by daynin · Pull Request #19084 · nodejs/node · GitHub

It fixes an error which occurs after running this test:

node benchmark/url/url-searchparams-read.js

Error:

internal/url.js:983
      throw new errors.TypeError('ERR_INVALID_THIS', 'URLSearchParams');
      ^

TypeError [ERR_INVALID_THIS]: Value of "this" must be of type URLSearchParams
    at get (internal/url.js:983:13)
    at main (/Users/daynin/Documents/projects/node/benchmark/url/url-searchparams-read.js:21:5)
    at Benchmark.process.nextTick (/Users/daynin/Documents/projects/node/benchmark/common.js:34:28)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)
    at Function.Module.runMain (module.js:678:11)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3

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

Affected core subsystem(s)

benchmark

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 fn by calling params[method]

@BridgeAR tell me pls, how can I add a test for a benchmark?

E.g. https://github.com/nodejs/node/blob/master/test/parallel/test-benchmark-path.js

@BridgeAR

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?

Done

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...

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

Seems like you're passing in method="get" given the quotation marks in the error? Instead pass in method=get.

@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

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

Not a fan of the churn for this change but the code itself looks good to me.

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

LGTM but needs a typo fix-up before landing.

Typo? Should be href, I think.

@apapirovski yeah, of course

@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

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

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 <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Rename different parameters with the same names to make it possible to run tests for url benchmarks. PR-URL: #19084 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <ruben@bridgewater.de> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to 8.x? If so, a separate backport PR is needed

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. url Issues and PRs related to the legacy built-in url module. labels Mar 2, 2018

BridgeAR reviewed Mar 2, 2018

View reviewed changes

benchmarks: fix benchmark for url

a4e954c

daynin force-pushed the fix-benchmark branch from b6ef43b to a4e954c Compare March 2, 2018 13:48

benchmark: add test for url benchmarks

57c86b6

BridgeAR reviewed Mar 2, 2018

View reviewed changes

daynin force-pushed the fix-benchmark branch 2 times, most recently from fbb327b to 98d406f Compare March 2, 2018 17:43

BridgeAR approved these changes Mar 2, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018

XadillaX approved these changes Mar 3, 2018

View reviewed changes

Leko approved these changes Mar 3, 2018

View reviewed changes

apapirovski approved these changes Mar 4, 2018

View reviewed changes

apapirovski removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 4, 2018

benchmark: rename different parameters

3cd0d7f

- 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

daynin force-pushed the fix-benchmark branch from 98d406f to 3cd0d7f Compare March 4, 2018 15:58

jasnell approved these changes Mar 5, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 6, 2018

BridgeAR closed this Mar 11, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

jasnell added the backport-requested-v8.x label Aug 17, 2018


		const runBenchmark = require('../common/benchmark');

		runBenchmark('url', ['n=1'], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 });

benchmarks: fix benchmark for url by daynin · Pull Request #19084 · nodejs/node · GitHub

Uh oh!

Uh oh!

benchmarks: fix benchmark for url - #19084

benchmarks: fix benchmark for url#19084daynin wants to merge 3 commits intonodejs:masternodejs/node:masterfrom daynin:fix-benchmarkdaynin/node:fix-benchmarkCopy head branch name to clipboard

Conversation

daynin commented Mar 2, 2018

Checklist

Affected core subsystem(s)

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

daynin commented Mar 2, 2018

Uh oh!

daynin commented Mar 2, 2018

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

daynin commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daynin commented Mar 2, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 2, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daynin commented Mar 6, 2018

Uh oh!

apapirovski commented Mar 6, 2018

Uh oh!

BridgeAR commented Mar 11, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benchmarks: fix benchmark for url#19084
daynin wants to merge 3 commits into
nodejs:masterfrom
daynin:fix-benchmark