GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||
GeezerVison is showing github.com — More URLSearchParams improvements by TimothyGu · Pull Request #10399 · nodejs/node Conversation
|
|
We could also consider exporting URLSearchParams from the querystring module. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
dd8fecb to
06ba384
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I haven't benched Object.create(null) with V8 5.4 to see if it performs any better now, but you might want to consider using the same trick used in querystring.js for better performance.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm concerned about the performance hits here. Is an array absolutely necessary? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mscdex, first off this API is still considered experimental. Performance is not a first priority. Second, the benchmarks that really matter (iteration and reading) all show significant performance increase. Why I said "really matter" is because the plan is to eventually stop using querystring module, and because of that the use of conversion functions like var params = new URLSearchParams(); params.append('a', 'a'); params.append('b', 'b'); params.append('a', 'c'); for (var [ name, val ] of params) { console.log(name, val); } With object With array (correct) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
06ba384 to
22fb822
Compare
|
We will definitely need to put some work into the performance very soon, but I agree that the focus on standards compliance first is the right approach. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Look at https://github.com/nodejs/node/blob/master/benchmark/common.js#L211 .. there is a utility method that takes care of the V8 optimization bits.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The v8ForceOptimization function forces the this context of the function to be null, which means that functions that depend on this to be set properly, like params.forEach cannot be used with v8ForceOptimization.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
why do it like this? why not ['forEach', 'iterator']?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This was from the existing url/new-url-parse.js benchmark.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
22fb822 to
009a147
Compare
|
@TimothyGu Can you rebase this onto the master? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@joyeecheung, I have just done so. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
ddfc3bb to
7df717c
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think this file should be in benchmark/fixtures?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm did not notice that before. Thanks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The convention seems to be to include this type of data in all benchmark files, so I did that instead.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
createBenchmark now takes a third argument for flags that will be turned on when running main(see the guide), so I think this can be replaced by passing a { flags: ['--allow_natives_syntax'] } to createBenchmark as the third argument.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
To make OptimizeFunctionOnNextCall work properly, we should call it after making the first call to params.forEach(it needs enough type info to optimize, which can be gathered in the first call).
BTW I am not sure we do need to do this step, because the optimizing compiler doesn't necessarily have enough type feedback to properly optimize this with just one call. I believe this conventions is to prevent the OSR(on-stack replacement) from affecting the result, but if the compiler doesn't have enough type info during the first optimization, it might still kick in again in the loop.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I guess I do have to look into these calls more carefully, esp. wrt. their necessity.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Testing against a noop can easily lead to weird results when the optimizing compiler can do JIT optimizations. This line is a loop invariant so could be code-motioned, leaving us timing an empty loop(which could explain the 1862.07 % improvement, can't be sure without looking into the IR). Maybe we can do something with i and the params in the loop(thus breaking the invariant), and add another case with for-in?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I believe it is the fact that we now store an array so no further processing (conversion from object to array) is needed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
When callback is noop, it is possible for the optimizing compiler to eliminate callback.call(thisArg, value, key, this) completely (again, can't be sure without looking into IR, but it's possible). Stopping reassigning pairs definitely could contribute to it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added a noDead variable similar to your benchmark PR. Also note the increased performance of iterators as well, as iterators do not have a callback.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
+1 for better spec compliance. I am not very sure about the benchmark results without looking into IR but we can improve them later, no reason to block this(especially when this is blocking others like #10801 and #10635). Aside: the micro benchmarks under |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/5866/ Anything else that need to be addressed other than the benchmark stuff? @jasnell |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Other than that I think this is good to go |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
7df717c to
5d67048
Compare
5d67048 to
87f6c3a
Compare
2900381 to
87f6c3a
Compare
|
@jasnell, any other things I need to change before this can be applied? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jasnell, ping. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed 98bb65f |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
- add some benchmarks for URLSearchParams - change URLSearchParams backing store to an array - add custom inspection for URLSearchParams and its iterators PR-URL: #10399 Reviewed-By: James M Snell <[email protected]>
- add some benchmarks for URLSearchParams - change URLSearchParams backing store to an array - add custom inspection for URLSearchParams and its iterators PR-URL: nodejs#10399 Reviewed-By: James M Snell <[email protected]>
- add some benchmarks for URLSearchParams - change URLSearchParams backing store to an array - add custom inspection for URLSearchParams and its iterators PR-URL: nodejs#10399 Reviewed-By: James M Snell <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.