FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

benchmark: unify input to url-related benchmarks by joyeecheung · Pull Request #11264 · nodejs/node · GitHub

/ node Public

benchmark: unify input to url-related benchmarks - #11264

Closed
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:unify-url-bench
ConversationCommits (3)ChecksFiles changed
Closed

benchmark: unify input to url-related benchmarks#11264
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:unify-url-bench

Conversation

joyeecheung commented Feb 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 9, 2017
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
Comment thread benchmark/querystring/querystring-parse.js Outdated

mscdex Feb 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Duplicate assignment

joyeecheung Feb 10, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ooop, thanks for catching that.

mscdex reviewed Feb 9, 2017
View reviewed changes
Comment thread benchmark/url/whatwg-url-properties.js Outdated

mscdex Feb 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why was this removed?

joyeecheung Feb 10, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

There's a typo in the second commit message.

joyeecheung commented Feb 12, 2017

Copy link
Copy Markdown
Member Author

@mscdex Thanks for the review, updated, PTAL.

joyeecheung commented Feb 12, 2017

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Feb 13, 2017

Copy link
Copy Markdown
Member Author

@mscdex Can I have a LGTM please? Thanks!

joyeecheung commented Feb 15, 2017

Copy link
Copy Markdown
Member Author

Rebased

watilde reviewed Feb 15, 2017
View reviewed changes
Comment thread 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'

watilde Feb 15, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

joyeecheung Feb 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex commented Feb 15, 2017

Copy link
Copy Markdown
Contributor

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

mscdex commented Feb 15, 2017

Copy link
Copy Markdown
Contributor

CI is green, LGTM.

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 commented Feb 15, 2017

Copy link
Copy Markdown
Member

Landed in e571fd4...7ee9504

jasnell closed this Feb 15, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
benchmark: move url data to fixtures
b9bc94f 
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>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 16, 2017
benchmark: refactor whatwg-url-properties
bcc8111 
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>
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
Closed
4 tasks
joyeecheung deleted the unify-url-bench branch February 19, 2017 17:44
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

watilde watilde left review comments
jasnell jasnell approved these changes
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. querystring Issues and PRs related to the built-in querystring module. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL