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

daynin · 2018-03-02T13:28:03Z

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

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?

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.

daynin · 2018-03-02T13:48:52Z

@BridgeAR I replaced fn by calling params[method]

daynin · 2018-03-02T13:55:02Z

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

BridgeAR · 2018-03-02T13:59:43Z

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

daynin · 2018-03-02T15:52:49Z

@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

BridgeAR · 2018-03-02T16:10:09Z

test/parallel/test-benchmark-url.js

+
+const runBenchmark = require('../common/benchmark');
+
+runBenchmark('url', ['n=1'], { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 });


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.

daynin · 2018-03-02T17:37:00Z

@apapirovski @BridgeAR done

BridgeAR

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

BridgeAR · 2018-03-02T19:13:25Z

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

apapirovski

LGTM but needs a typo fix-up before landing.

apapirovski · 2018-03-04T12:56:40Z

test/parallel/test-benchmark-url.js

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

daynin · 2018-03-06T14:38:19Z

/cc @nodejs/collaborators

Hello! Run CI for this PR, please

apapirovski · 2018-03-06T17:43:05Z

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

BridgeAR · 2018-03-11T02:56:40Z

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>

jasnell · 2018-08-17T23:41:40Z

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 pending requests for changes, 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 pending requests for changes, 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 pending requests for changes, 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 });

Uh oh!

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