Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Conversation

daynin
Copy link
Contributor

@daynin daynin commented Mar 2, 2018

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

@nodejs-github-bot 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
Copy link
Member

@BridgeAR BridgeAR left a 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.

@daynin
Copy link
Contributor Author

daynin commented Mar 2, 2018

@BridgeAR I replaced fn by calling params[method]

@daynin
Copy link
Contributor Author

daynin commented Mar 2, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

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

@daynin
Copy link
Contributor Author

daynin commented Mar 2, 2018

@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


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

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

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

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@daynin
Copy link
Contributor Author

daynin commented Mar 2, 2018

@apapirovski @BridgeAR done

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

@BridgeAR BridgeAR left a 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.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

@BridgeAR 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
Copy link
Member

@apapirovski apapirovski left a 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.

'accessMethod=get',
'type=short',
'searchParam=noencode',
'herf=short',
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski yeah, of course

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski fixed

@apapirovski 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
- 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
Copy link
Contributor Author

daynin commented Mar 6, 2018

/cc @nodejs/collaborators

Hello! Run CI for this PR, please

@apapirovski
Copy link
Member

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

@BridgeAR 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
Copy link
Member

Landed in f3257dd 🎉

@BridgeAR BridgeAR closed this Mar 11, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 11, 2018
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>
targos pushed a commit that referenced this pull request Mar 17, 2018
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>
@targos targos mentioned this pull request Mar 18, 2018
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
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>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
Copy link
Member

jasnell commented Aug 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants