http,https: avoid instanceof for WHATWG URL#12983
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added http 
nodejs-github-bot added http 
lib/https.js Outdated
There was a problem hiding this comment.
Would a require('internal/url').isURL() utility function be worthwhile to make this more convenient? Or would that kill any perf gain? I imagine wanting to use similar checks elsewhere.
There was a problem hiding this comment.
Ha! I thought the question sounded familiar as I was typing it ;-)
TimothyGu left a comment
TimothyGu left a comment There was a problem hiding this comment.
It's unfortunate we can't write an isURL without performance regression, but 🤷♂️
lib/_http_client.js Outdated
There was a problem hiding this comment.
I am now wondering if this way of checking URLs is a bit hard to understand for someone who don't know the implementation details of URL...
There was a problem hiding this comment.
There's not a whole lot that can be done about that. Using a helper function as previously noted incurs a measurable performance hit.
mscdexforce-pushed the http-client-avoid-instanceof-url branch 2 times, most recently from 43954e6 to 41ea7e9Compare
mscdex commented May 22, 2017
CI before landing: https://ci.nodejs.org/job/node-test-pull-request/8234/ |
PR-URL: nodejs#12983 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
mscdexforce-pushed the http-client-avoid-instanceof-url branch from 41ea7e9 to ed36565Compare
PR-URL: #12983 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: #12983 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
9 participants
Benchmark results:
CI: https://ci.nodejs.org/job/node-test-pull-request/8025/
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)