benchmark: unify input to url-related benchmarks #11264

joyeecheung · 2017-02-09T16:45:23Z

This PR moves common input to url-related benchmarks into fixtures and removes duplicate benchmarks

url-parse.js is covered by legacy-vs-whatwg-url-parse.js with method="legacy"
stringifier benchmark in whatwg-url-properties.js is covered by legacy-vs-whatwg-url-serialize.js with method="legacy" (it is still worth keeping because legacy-vs-whatwg-url-get-prop.js benchmarks getting all properties at once while whatwg-url-properties.js benchmarks getting/setting individual property)

Some of the search params cases are taken from #11234 per suggestion from #11170 (review)

cc @nodejs/url

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

benchmark, url

mscdex · 2017-02-09T21:54:54Z

benchmark/querystring/querystring-parse.js

-                  'foo=ghi&foo=jkl&foo=mno&foo=pqr&foo=stu&foo=vwxyz',
-  manypairs: 'a&b&c&d&e&f&g&h&i&j&k&l&m&n&o&p&q&r&s&t&u&v&w&x&y&z'
-};
+var inputs = inputs = require('../fixtures/url-inputs.js').searchParams;


Duplicate assignment

Ooop, thanks for catching that.

mscdex · 2017-02-09T21:58:42Z

benchmark/url/whatwg-url-properties.js

    case 'origin':
    case 'searchParams':
      get(n, url, prop);
      break;
-    case 'toString':


Why was this removed?

This is covered in legacy-vs-whatwg-url-serialize.js with method="whatwg", also href is an alias of toString so we can just look at the results of href.

mscdex · 2017-02-09T21:59:31Z

There's a typo in the second commit message.

joyeecheung · 2017-02-12T06:03:14Z

@mscdex Thanks for the review, updated, PTAL.

joyeecheung · 2017-02-12T06:13:26Z

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

joyeecheung · 2017-02-13T13:20:32Z

@mscdex Can I have a LGTM please? Thanks!

joyeecheung · 2017-02-15T12:09:24Z

Rebased

watilde · 2017-02-15T12:42:44Z

benchmark/fixtures/url-inputs.js

+  ws: 'ws://localhost:9229/f46db715-70df-43ad-a359-7f9949f39868',
+  javascript: 'javascript:alert("node is awesome");',
+  percent: 'https://%E4%BD%A0/foo',
+  dot: 'https://example.org/./a/../b/./c'


Could we add mailto:domain@example.com into here as an example case of mailto protocol?

That should probably just hit the same code paths as 'javascript:alert("node is awesome");'

mscdex · 2017-02-15T13:11:30Z

Lint: https://ci.nodejs.org/job/node-test-linter/7083/

mscdex · 2017-02-15T15:31:51Z

CI is green, LGTM.

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2017-02-15T17:15:54Z

Landed in e571fd4...7ee9504

PR-URL: nodejs#11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Backport-of: nodejs#11264

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Backport-of: #11264

nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 9, 2017

cjihrig force-pushed the master branch from 5ff364b to 89f8ef2 Compare February 9, 2017 16:47

joyeecheung added whatwg-url Issues and PRs related to the WHATWG URL implementation. querystring Issues and PRs related to the built-in querystring module. url Issues and PRs related to the legacy built-in url module. labels Feb 9, 2017

mscdex reviewed Feb 9, 2017

View reviewed changes

jasnell approved these changes Feb 10, 2017

View reviewed changes

joyeecheung force-pushed the unify-url-bench branch from 7a15a68 to 95157e9 Compare February 12, 2017 06:02

joyeecheung added 3 commits February 15, 2017 19:52

benchmark: move url data to fixtures

54fbd5d

benchmark: refactor whatwg-url-properties

93ee651

benchmark: clean up legacy url benchmarks

b486814

joyeecheung force-pushed the unify-url-bench branch from 95157e9 to b486814 Compare February 15, 2017 12:09

watilde reviewed Feb 15, 2017

View reviewed changes

jasnell pushed a commit that referenced this pull request Feb 15, 2017

benchmark: move url data to fixtures

e571fd4

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell pushed a commit that referenced this pull request Feb 15, 2017

benchmark: refactor whatwg-url-properties

d8ac08a

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell pushed a commit that referenced this pull request Feb 15, 2017

benchmark: clean up legacy url benchmarks

7ee9504

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell closed this Feb 15, 2017

TimothyGu pushed a commit to TimothyGu/node that referenced this pull request Feb 18, 2017

benchmark: clean up legacy url benchmarks

0127450

Backport-of: nodejs#11264

TimothyGu mentioned this pull request Feb 18, 2017

(v7.x backport) URL-related changes #11457

Closed

4 tasks

joyeecheung deleted the unify-url-bench branch February 19, 2017 17:44

italoacasas added the dont-land-on-v7.x label Feb 20, 2017

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017

benchmark: clean up legacy url benchmarks

83dad76

Backport-of: nodejs#11264

italoacasas pushed a commit that referenced this pull request Feb 22, 2017

benchmark: move url data to fixtures

7a3553f

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

italoacasas pushed a commit that referenced this pull request Feb 22, 2017

benchmark: refactor whatwg-url-properties

678ea11

PR-URL: #11264 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

italoacasas pushed a commit that referenced this pull request Feb 22, 2017

benchmark: clean up legacy url benchmarks

5a81031

Backport-of: #11264

jasnell added dont-land-on-v4.x labels Mar 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

benchmark: unify input to url-related benchmarks #11264

benchmark: unify input to url-related benchmarks #11264

joyeecheung commented Feb 9, 2017 •

edited

Loading

mscdex Feb 9, 2017

joyeecheung Feb 10, 2017

mscdex Feb 9, 2017

joyeecheung Feb 10, 2017

mscdex commented Feb 9, 2017

joyeecheung commented Feb 12, 2017

joyeecheung commented Feb 12, 2017

joyeecheung commented Feb 13, 2017

joyeecheung commented Feb 15, 2017

watilde Feb 15, 2017

joyeecheung Feb 15, 2017

mscdex commented Feb 15, 2017

mscdex commented Feb 15, 2017

jasnell commented Feb 15, 2017

benchmark: unify input to url-related benchmarks #11264

benchmark: unify input to url-related benchmarks #11264

Conversation

joyeecheung commented Feb 9, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex Feb 9, 2017

Choose a reason for hiding this comment

joyeecheung Feb 10, 2017

Choose a reason for hiding this comment

mscdex Feb 9, 2017

Choose a reason for hiding this comment

joyeecheung Feb 10, 2017

Choose a reason for hiding this comment

mscdex commented Feb 9, 2017

joyeecheung commented Feb 12, 2017

joyeecheung commented Feb 12, 2017

joyeecheung commented Feb 13, 2017

joyeecheung commented Feb 15, 2017

watilde Feb 15, 2017

Choose a reason for hiding this comment

joyeecheung Feb 15, 2017

Choose a reason for hiding this comment

mscdex commented Feb 15, 2017

mscdex commented Feb 15, 2017

jasnell commented Feb 15, 2017

joyeecheung commented Feb 9, 2017 •

edited

Loading