benchmark: pre-generate data set for URL benchmarks and use WPT test data#24302
Closed
joyeecheung wants to merge 3 commits intonodejs:masterfrom
Closed
benchmark: pre-generate data set for URL benchmarks and use WPT test data#24302joyeecheung wants to merge 3 commits intonodejs:masterfrom
joyeecheung wants to merge 3 commits intonodejs:masterfrom
Conversation
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js`
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case.
joyeecheung
added
the
whatwg-url
nodejs-github-bot
added
benchmark
Member
Author
|
CI: https://ci.nodejs.org/job/node-test-pull-request/18529/ cc @TimothyGu @nodejs/url |
3 tasks
mscdex
reviewed
Nov 11, 2018
| .startsWith('http://')); | ||
| const bench = common.createBenchmark(main, { | ||
| len: [1, 8, 16, 32, 64, 128], | ||
| n: [1e6] |
Contributor
There was a problem hiding this comment.
Why is n being removed in general in this PR? If we're not going to be looping over the same input for each input, then we should at least loop n times over the entire set of inputs.
Member
Author
There was a problem hiding this comment.
@mscdex That should already been covered by the combination e and --runs (of the compare.js): the size of the input is dependent on e so to get a size of 1e6 we can just set e to roughly Math.log(1e6 / 200, 2) (all the data are pre-generated, so when e is 1 and type is long it will iterate over the same kind of string 400 times. Only the type wpt gives you a nonhomogenous array), but I don't think we actually need that many iterations for a single benchmark run, the n was set when Crankshaft existed, and currently I don't observe a significance in the value of the n once it's large enough (by default it's roughly 800/400 with/without base). Instead we can just use --runs to get more samples, but the default 30 is already a pretty good sample size (at least it's the magic number taught in statistics textbooks). So with the default setting for each input you run through the same code roughly 30 * 400 times with the same binary
Member
Author
|
@mscdex BTW this is the benchmark results I got for #24218 with this benchmark after setting See resultsFor comparison, with the old benchmark the accuracy isn't really any different from when See results |
Member
Author
|
Added a type check for the argument of |
Member
Author
|
Ping @nodejs/url @nodejs/performance can I have some reviews please? As the data in #24302 (comment) demonstrates, this should not affect the accuracy of the benchmarks while providing more options for the data set. |
joyeecheung
added
the
author ready
Member
Author
jasnell
approved these changes
Nov 19, 2018
Member
|
Since this affects benchmarks, let's run the benchmark CI job on it: https://ci.nodejs.org/job/node-test-commit-custom-suites/766/ |
Member
Author
|
Landed in 5f25dd1...8450d11, thanks! |
joyeecheung
added a commit
that referenced
this pull request
Nov 19, 2018
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung
added a commit
that referenced
this pull request
Nov 19, 2018
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
targos
pushed a commit
that referenced
this pull request
Nov 19, 2018
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
targos
pushed a commit
that referenced
this pull request
Nov 19, 2018
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg
pushed a commit
that referenced
this pull request
Nov 28, 2018
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg
pushed a commit
that referenced
this pull request
Nov 28, 2018
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Dec 7, 2018
Member
|
@joyeecheung is this something you think should be backported to |
refack
pushed a commit
to refack/node
that referenced
this pull request
Jan 14, 2019
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: nodejs#24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
refack
pushed a commit
to refack/node
that referenced
this pull request
Jan 14, 2019
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: nodejs#24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
benchmark: pre-generate data set for URL benchmarks
This patch:
common.bakeUrlDatawhich can be used to pre-generatethe data set for the URL benchmarks to loop through instead of
looping over a constant.
in the input
benchmark/fixtures/url-inputs.jstobenchmark/common.jsbenchmark: support URL inputs in create-clientrequest
This patch adds the option in the create-clientrequest benchmark
to accept URL inputs (as strings or as URL objects) so we can
measure the impact of URL parsing in a more sophisticated use case.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes