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

url: improve parsing speed #102

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

The url.parse() function now checks whether an escapable character is
in the URL before trying to escape it.

PR-URL: nodejs/node-v0.x-archive#8638

@CGavrila Would you be willing to sign the commit with your real name?

CGavrila and others added 2 commits December 6, 2014 20:39
The url.parse() function now checks whether an escapable character is
in the URL before trying to escape it.

PR-URL: nodejs/node-v0.x-archive#8638
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@piscisaureus piscisaureus added the wip Issues and PRs that are still a work in progress. label Dec 6, 2014
@indutny
Copy link
Member

indutny commented Dec 8, 2014

Please tag it, @caineio

@caineio caineio added need info and removed wip Issues and PRs that are still a work in progress. need info labels Dec 8, 2014
@bnoordhuis
Copy link
Member Author

@chrisdickinson Maybe you can review b7fc957? That one can go in without having to wait for the OPR (original pull requester; yes, I made that up on the spot.)

@chrisdickinson
Copy link
Contributor

@bnoordhuis done and done. Had one request + a italicized verdant hope for a v8 feature, otherwise b7fc957 looks good.

bnoordhuis added a commit that referenced this pull request Dec 9, 2014
Based on the ad-hoc benchmark from nodejs/node-v0.x-archive#8638 plus an additional
benchmark for user:pass auth URLs.

PR-URL: #102
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
@bnoordhuis
Copy link
Member Author

Cheers Chris, incorporated the feedback and landed in 21a679a.

I wish there was a way we could tell v8 to make sure not to OSR-inline the url.parse since that could skew the results.

I suppose doing a few url.parse() trial runs, then calling %OptimizeFunctionOnNextCall(url.parse) before the actual benchmark might do it. I'll see if I can make it work.

@bnoordhuis
Copy link
Member Author

Closing, this landed by way of the joyent/v0.12 merge in 94e1475.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. 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