benchmark: improve WHATWG URL benchmarks #10678
Conversation
nodejs-github-bot
added
benchmark
joyeecheung
added
url
|
BTW, on Results of `url.format(old)` v.s. `new.toString()`url/new-url-properties.js n=100000 method="old" prop="stringifier" type="long": 1,931,178.2529862437 url/new-url-properties.js n=100000 method="new" prop="stringifier" type="long": 1,644,836.5873766062 url/new-url-properties.js n=100000 method="old" prop="stringifier" type="short": 2,211,006.4253615527 url/new-url-properties.js n=100000 method="new" prop="stringifier" type="short": 1,520,694.4232835593 url/new-url-properties.js n=100000 method="old" prop="stringifier" type="idn": 2,198,824.723787486 url/new-url-properties.js n=100000 method="new" prop="stringifier" type="idn": 5,876,998.649700791 url/new-url-properties.js n=100000 method="old" prop="stringifier" type="auth": 1,617,199.3157888448 url/new-url-properties.js n=100000 method="new" prop="stringifier" type="auth": 4,042,281.45556896 url/new-url-properties.js n=100000 method="old" prop="stringifier" type="special": 1,313,069.9158389152 url/new-url-properties.js n=100000 method="new" prop="stringifier" type="special": 1,736,090.9471381314 Results of `url.protocol`url/new-url-properties.js n=100000 method="old" prop="protocol" type="long": 11,182,667.759586062 url/new-url-properties.js n=100000 method="new" prop="protocol" type="long": 6,225,058.524887721 url/new-url-properties.js n=100000 method="old" prop="protocol" type="short": 11,167,068.048315883 url/new-url-properties.js n=100000 method="new" prop="protocol" type="short": 5,045,771.708902972 url/new-url-properties.js n=100000 method="old" prop="protocol" type="idn": 10,646,324.521476991 url/new-url-properties.js n=100000 method="new" prop="protocol" type="idn": 5,635,780.578998681 url/new-url-properties.js n=100000 method="old" prop="protocol" type="auth": 9,157,233.266506897 url/new-url-properties.js n=100000 method="new" prop="protocol" type="auth": 6,366,295.507394388 url/new-url-properties.js n=100000 method="old" prop="protocol" type="special": 11,233,445.832548864 url/new-url-properties.js n=100000 method="new" prop="protocol" type="special": 6,336,287.96553262 Results of auth(`old.auth` v.s. `new.username + ':' + new.password`)url/new-url-properties.js n=100000 method="old" prop="auth" type="long": 9,825,048.312218813 url/new-url-properties.js n=100000 method="new" prop="auth" type="long": 35,201,017.1685921 url/new-url-properties.js n=100000 method="old" prop="auth" type="short": 9,592,973.032138282 url/new-url-properties.js n=100000 method="new" prop="auth" type="short": 38,615,659.499471545 url/new-url-properties.js n=100000 method="old" prop="auth" type="idn": 10,031,574.380362188 url/new-url-properties.js n=100000 method="new" prop="auth" type="idn": 36,363,623.14050067 url/new-url-properties.js n=100000 method="old" prop="auth" type="auth": 8,806,970.75257855 url/new-url-properties.js n=100000 method="new" prop="auth" type="auth": 15,470,957.608957313 url/new-url-properties.js n=100000 method="old" prop="auth" type="special": 9,792,467.26206826 url/new-url-properties.js n=100000 method="new" prop="auth" type="special": 35,558,210.568611346 Results of `url.host`url/new-url-properties.js n=100000 method="old" prop="host" type="long": 11,111,432.108038675 url/new-url-properties.js n=100000 method="new" prop="host" type="long": 4,903,943.27549966 url/new-url-properties.js n=100000 method="old" prop="host" type="short": 11,261,911.86495346 url/new-url-properties.js n=100000 method="new" prop="host" type="short": 6,436,216.642537291 url/new-url-properties.js n=100000 method="old" prop="host" type="idn": 11,836,698.072583107 url/new-url-properties.js n=100000 method="new" prop="host" type="idn": 6,072,991.651033267 url/new-url-properties.js n=100000 method="old" prop="host" type="auth": 11,501,686.664840966 url/new-url-properties.js n=100000 method="new" prop="host" type="auth": 5,357,784.547517147 url/new-url-properties.js n=100000 method="old" prop="host" type="special": 8,974,045.803170817 url/new-url-properties.js n=100000 method="new" prop="host" type="special": 5,829,677.008494248 Results of `url.hostname`url/new-url-properties.js n=100000 method="old" prop="hostname" type="long": 11,223,056.10776109 url/new-url-properties.js n=100000 method="new" prop="hostname" type="long": 6,430,740.793044099 url/new-url-properties.js n=100000 method="old" prop="hostname" type="short": 11,458,170.173764298 url/new-url-properties.js n=100000 method="new" prop="hostname" type="short": 5,221,261.939329145 url/new-url-properties.js n=100000 method="old" prop="hostname" type="idn": 9,601,310.386841595 url/new-url-properties.js n=100000 method="new" prop="hostname" type="idn": 4,953,392.538922767 url/new-url-properties.js n=100000 method="old" prop="hostname" type="auth": 8,185,137.9993803855 url/new-url-properties.js n=100000 method="new" prop="hostname" type="auth": 4,500,329.131571037 url/new-url-properties.js n=100000 method="old" prop="hostname" type="special": 9,691,541.493947245 url/new-url-properties.js n=100000 method="new" prop="hostname" type="special": 5,756,141.083017945 Results of `url.port`url/new-url-properties.js n=100000 method="old" prop="port" type="long": 11,342,818.424956651 url/new-url-properties.js n=100000 method="new" prop="port" type="long": 6,128,693.993696026 url/new-url-properties.js n=100000 method="old" prop="port" type="short": 11,457,772.3798939 url/new-url-properties.js n=100000 method="new" prop="port" type="short": 5,535,808.710794296 url/new-url-properties.js n=100000 method="old" prop="port" type="idn": 11,054,064.767534124 url/new-url-properties.js n=100000 method="new" prop="port" type="idn": 5,424,287.79101304 url/new-url-properties.js n=100000 method="old" prop="port" type="auth": 10,687,846.272859434 url/new-url-properties.js n=100000 method="new" prop="port" type="auth": 6,240,790.93287967 url/new-url-properties.js n=100000 method="old" prop="port" type="special": 10,964,146.473541267 url/new-url-properties.js n=100000 method="new" prop="port" type="special": 6,244,516.143978809 Results of `url.pathname`url/new-url-properties.js n=100000 method="old" prop="pathname" type="long": 9,257,141.259624766 url/new-url-properties.js n=100000 method="new" prop="pathname" type="long": 2,264,299.754885023 url/new-url-properties.js n=100000 method="old" prop="pathname" type="short": 9,944,587.762528416 url/new-url-properties.js n=100000 method="new" prop="pathname" type="short": 1,853,560.5944435555 url/new-url-properties.js n=100000 method="old" prop="pathname" type="idn": 10,553,184.196437815 url/new-url-properties.js n=100000 method="new" prop="pathname" type="idn": 4,409,727.735913048 url/new-url-properties.js n=100000 method="old" prop="pathname" type="auth": 9,023,485.60645764 url/new-url-properties.js n=100000 method="new" prop="pathname" type="auth": 4,356,949.349461714 url/new-url-properties.js n=100000 method="old" prop="pathname" type="special": 11,092,131.242096856 url/new-url-properties.js n=100000 method="new" prop="pathname" type="special": 2,065,724.793938271 Results of `url.search`url/new-url-properties.js n=100000 method="old" prop="search" type="long": 11,041,101.93730487 url/new-url-properties.js n=100000 method="new" prop="search" type="long": 6,280,379.767028056 url/new-url-properties.js n=100000 method="old" prop="search" type="short": 8,030,524.989146745 url/new-url-properties.js n=100000 method="new" prop="search" type="short": 5,765,022.524519505 url/new-url-properties.js n=100000 method="old" prop="search" type="idn": 10,384,961.163879983 url/new-url-properties.js n=100000 method="new" prop="search" type="idn": 5,369,139.047815405 url/new-url-properties.js n=100000 method="old" prop="search" type="auth": 8,907,717.031403532 url/new-url-properties.js n=100000 method="new" prop="search" type="auth": 4,778,068.512628841 url/new-url-properties.js n=100000 method="old" prop="search" type="special": 10,703,034.14963591 url/new-url-properties.js n=100000 method="new" prop="search" type="special": 6,278,603.354821017 Results of `url.hash`url/new-url-properties.js n=100000 method="old" prop="hash" type="long": 11,636,972.990585689 url/new-url-properties.js n=100000 method="new" prop="hash" type="long": 5,642,725.316702191 url/new-url-properties.js n=100000 method="old" prop="hash" type="short": 9,114,039.695106206 url/new-url-properties.js n=100000 method="new" prop="hash" type="short": 5,862,102.824214588 url/new-url-properties.js n=100000 method="old" prop="hash" type="idn": 10,639,332.37763757 url/new-url-properties.js n=100000 method="new" prop="hash" type="idn": 5,664,185.507966138 url/new-url-properties.js n=100000 method="old" prop="hash" type="auth": 11,295,987.518385632 url/new-url-properties.js n=100000 method="new" prop="hash" type="auth": 5,518,077.082570574 url/new-url-properties.js n=100000 method="old" prop="hash" type="special": 7,582,483.583354356 url/new-url-properties.js n=100000 method="new" prop="hash" type="special": 5,381,531.766912944 |
|
I think using names like 'original' and 'whatwg' or something similar might be more descriptive? |
|
I name it this way because there is a |
|
I'd be good with renaming those in this PR |
|
I think |
| var obj = new URL(input); | ||
| var noDead = ''; | ||
|
|
||
| if (prop === 'stringifier') { |
There was a problem hiding this comment.
I'd be happier with the wording "serializer" considering that's the spec definition, but strinigifier works too.
There was a problem hiding this comment.
I grabbed that name from the API definition in the spec https://url.spec.whatwg.org/#URL-stringification-behavior
| if (prop === 'stringifier') { | ||
| bench.start(); | ||
| for (var i = 0; i < n; i += 1) { | ||
| noDead = obj.toString(); |
There was a problem hiding this comment.
This is a bit unfair as the original url stores the query string unparsed, while WHATWG URL serializer has to serialize the searchParams as well.
There was a problem hiding this comment.
It is not about fairness really, because they look the same from end users' perspective
| }; | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| type: Object.keys(inputs), |
There was a problem hiding this comment.
Grab that from new-url-parse, I don't have an opinion about this but I guess a little bit consistency wouldn't hurt?
| type: Object.keys(inputs), | ||
| prop: ['stringifier', 'protocol', 'auth', 'host', 'hostname', 'port', | ||
| 'pathname', 'search', 'hash'], | ||
| method: ['old', 'new'], |
There was a problem hiding this comment.
I know this is a vestige of new-url-parse, but I think Node.js/legacy vs WHATWG is clearer.
Also the prop name method doesn't really convey the difference here, I think.
There was a problem hiding this comment.
Yeah legacy and whatwg would be much clearer! Thanks for the suggestion.
Can't think of a better alternative of "method" though...
| bench.end(n); | ||
| } | ||
|
|
||
| assert.ok(!noDead || noDead.length > 0); |
There was a problem hiding this comment.
Just a habit, make sure it won't get dead code elimination(V8 can't do that for these cases at the moment but..A smarter JIT usually would
There was a problem hiding this comment.
Ooops, just took a look into the IR of method="old" ones, and it looks like the whole section inside useOld got elimiated :/ Will update to move the check into the main function. (The new ones aren't affected because...Crankshaft can't figure this out when there are getters involved I guess)
| const common = require('../common.js'); | ||
| const url = require('url'); | ||
| const URL = url.URL; | ||
| const v8 = require('v8'); |
There was a problem hiding this comment.
Good catch! Forgot to run the linter :P
|
IMO the file name should also make it clear that this benchmark concerns reading properties only, and TBH the entire benchmark is a bit pointless as it's comparing straight JS property access with the performance of getter functions. |
|
@TimothyGu Yeah it could seem a little unfair because optimization usually don't go hand in hand with specs, but IMO if users want to switch, they wouldn't care about the internals, but they would want to know about the performance impact. If the performance impact is inevitable, at least we can know how much it would be, and do a little bit expectation management. As said in the PR message, I agree the current way this benchmark is implemented could be too "micro"(like most of them have loop invariants that just don't get optimized by the JIT because they jump too many hoops :P), so this is more like a first attempt. Maybe the operations in each cycle can be a little bit more sophisticated? |
joyeecheung
force-pushed
the
url-properties-benchmark
branch
from
54d0ebc
to
f45b900
Compare
joyeecheung
force-pushed
the
url-properties-benchmark
branch
from
f45b900
to
4aeec05
Compare
|
@jasnell @TimothyGu @mscdex I've refactored the parse benchmark a bit, and separated the serialization benchmark out. Also stopped benchmarking individual properties because I find them too "micro" to get right (and turns out Crankshaft can't optimize |
| idn: 'http://你好你好', | ||
| auth: 'https://user:pass@example.com/path?search=1', | ||
| special: 'file:///foo/bar/test/node.js' | ||
| }; |
There was a problem hiding this comment.
If we're including an IDN test, it would be worthwhile also testing a case that uses pct-encoded characters in the domain also
There was a problem hiding this comment.
Also, let's include a test that requires dot segment normalization.... e.g.
https://example.org/./a/../b/./c
There was a problem hiding this comment.
These should have zero impact on the getter performance but it's good to be complete
There was a problem hiding this comment.
Good idea. I'll add those to the inputs.
|
Couple of minor optional comments. LGTM |
|
|
||
| function useWHATWG(n, input) { | ||
| var obj = new URL(input); | ||
| var noDead = {}; |
There was a problem hiding this comment.
Why do these need to be initialized?
There was a problem hiding this comment.
Just a habit of initalizing everything...can switch to intializing the properties I think.
| var noDead = {}; | ||
| bench.start(); | ||
| for (var i = 0; i < n; i += 1) { | ||
| noDead = { |
There was a problem hiding this comment.
Instead of creating a new object every iteration, what about just pre-defining the properties in var noDead = { /* ... */ } and then just re-assigning the individual properties inside the loop?
There was a problem hiding this comment.
Yeah that would be better. I was not sure about this because some properties might be null, but come to think of it maybe I can just intialize them all to undefined.
joyeecheung
force-pushed
the
url-properties-benchmark
branch
from
4aeec05
to
81c28d1
Compare
joyeecheung
changed the title
|
@mscdex @jasnell Updated, PTAL. @TimothyGu Does this LGTY now? (See #10678 (comment)) |
| }; | ||
| bench.start(); | ||
| for (var i = 0; i < n; i += 1) { | ||
| noDead = { |
There was a problem hiding this comment.
This and the other instance below need to be changed yet to
noDead.protocol = obj.protocol;
noDead.auth = obj.auth;
// etc...There was a problem hiding this comment.
Ah, forgot about this bit. Thanks & fixed :).
joyeecheung
force-pushed
the
url-properties-benchmark
branch
from
81c28d1
to
31c6899
Compare
| throw new Error('Unknown method'); | ||
| } | ||
|
|
||
| assert.ok(noDead); |
There was a problem hiding this comment.
This and the return noDead in each function are unnecessary.
There was a problem hiding this comment.
Crankshaft is able to do code motion if we simply do var protocol = obj.protocol and leave us timing an empty loop in the legacy version(WHATWG version is not affected because V8's current limitation of closures affects the getters). At the moment looks like it's the fear of changing the hidden class of noDead makes it back off from doing code motion for the legacy implementation(by looking at the output of --trace_gvn), so keeping noDead not dead could be a little bit more future-proof.
Aside: another way would be to add some interface to bench so it can suspend and resume the timer at anytime, then we can have more flexibility when it comes to avoiding loop invariant code motion, like generating different inputs based on i for each cycle.
There was a problem hiding this comment.
If it's absolutely needed, then there should be some comments above each explaining things a bit, otherwise someone may come along and submit a PR to remove them without knowing.
There was a problem hiding this comment.
Agree, I will at some comments to the declaration of noDead in main.
| throw new Error('Unknown method'); | ||
| } | ||
|
|
||
| assert.ok(noDead); |
| throw new Error('Unknown method'); | ||
| } | ||
|
|
||
| assert.ok(noDead); |
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations
joyeecheung
force-pushed
the
url-properties-benchmark
branch
from
3ae7a3b
to
e0f6209
Compare
|
Squashed with new commit messages. CI: https://ci.nodejs.org/job/node-test-pull-request/5858/ |
|
Also: The new benchmark numbers
|
|
@mscdex Does this LGTY? |
|
LGTM |
|
Landed in d4b749b |
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations PR-URL: #10678 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations PR-URL: nodejs#10678 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations PR-URL: nodejs#10678 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations PR-URL: nodejs#10678 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
* add benchmark to compare the performance of getting url properties between the WHATWG URL and the legacy implementation * add benchmark to compare the performance of serializing urls between the WHATWG URL and the legacy implementation * refactor the benchmark for comparing parsing performance between the two implementations PR-URL: nodejs#10678 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Checklist
Affected core subsystem(s)
benchmark, url
This adds benchmark to compare the performance of WHATWG URL
implementation with that of the old URL API regarding properties
and stringifiers(
url.formatv.s.URL#toString).The WHATWG URL implementation is still experimental but it might be helpful to start tracking the performance differences. The properties are kinda destined to be slower, because they need to invoke accessors and go through some code to match what the spec requires, but this can help us to see where we are at, and might be useful when we want to evangelise them(so people could see the performance improvments/hits before jumping in).
This is a first attempt and I think the current way to benchmark these properties could be too "micro". An alternative approach would be to test a set of operations in each benchmark cycle instead of doing only one thing in on cycle. And the setters could be benchmarked too(e.g. set and stringify in a cycle). I would like to see what we think about this.
@nodejs/url