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

Benchmark suite #1008

Merged
merged 14 commits into from
Sep 3, 2018
Merged

Benchmark suite #1008

merged 14 commits into from
Sep 3, 2018

Conversation

cellog
Copy link
Contributor

@cellog cellog commented Aug 30, 2018

This PR implements #983

It adds 2 new commands:

npm run setup:benchmarks
npm run benchmarks

setup:benchmarks is quite heavy, it builds each benchmark, and then creates a structure in benchmark/runs that contains a copy of each benchmark's build folder for each version of react-redux, but it only needs to be run once. It can probably be optimized, and then converted to a pre-benchmarks script. react-redux versions are specified by dropping minified react-redux.min.js into benchmarks/react-redux-versions pre-added are versions 4.4.0, 5.0.7, and the versions represented by #995 and #1000 PRs. New benchmarks can be added as subdirectories of benchmarks/sources with the one @markerikson ported named as jimbolla since it's based on his work.

To limit runs to specific react-redux versions, one can use the REDUX env variable with versions separated by colon as in:

REDUX=5.0.7:6.0-greg npm run benchmarks

To limit to specific benchmarks, use BENCHMARKS env variable as in:

BENCHMARKS=jimbolla npm run benchmarks

Note that the current version of the jimbolla benchmark appears not to work with react-redux 4.4.9, but I'm sure this can be easily fixed. Example output:

Gregs-MacBook-Pro:react-redux gregorybeaver$ npm run benchmarks

> [email protected] benchmarks /Users/gregorybeaver/Documents/GitHub/react-redux
> node ./benchmarks/runBenchmarks.js

Running benchmark jimbolla
  react-redux version: 4.4.9
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 5.0.7
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0-greg
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
  react-redux version: 6.0-mark
    Checking max FPS... (30 seconds)
    Running trace...    (30 seconds)
jimbolla results:
  --4.4.9:
  FPS (average, values):  0 ;  []
  Profile:  { categories: 
   { scripting: 22437.16899999231,
     rendering: 168.67900001257658,
     painting: 20.788999997079372 } }
G  --5.0.7:
  FPS (average, values):  18.666666666666668 ;  [ 1, 18, 19, 20, 19, 18, 19, 18, 19, 18, 19, 18, 19 ]
  Profile:  { categories: 
   { scripting: 16651.22200011462,
     rendering: 10659.14299992472,
     painting: 1436.54699999094 } }
  --6.0-greg:
  FPS (average, values):  24.736842105263158 ;  [ 3, 24, 25, 27, 25, 29, 26, 25, 26, 24, 25, 24, 26, 24, 23, 22, 24, 23, 25, 23 ]
  Profile:  { categories: 
   { scripting: 14569.813999965787,
     rendering: 11497.913000144064,
     painting: 2019.5350000336766 } }
  --6.0-mark:
  FPS (average, values):  17.25 ;  [ 1, 18, 19, 18, 19, 18, 17, 18, 17, 16, 17, 16, 17, 16, 17, 16, 17 ]
  Profile:  { categories: 
   { scripting: 18397.604999758303,
     rendering: 9224.441999822855,
     painting: 1241.0509999021888 } }

So a few known bugs to work out:

  • the first benchmark doesn't work with react-redux 4.4.9 it was working, with less than 1 fps, fixed so that if fps is that slow it shows 0 fps
  • the tracealyzed data isn't working quite yet

These are minor details, easily worked out. The heavy lifting is done, so now focus can begin on creating new benchmarks.

@markerikson
Copy link
Contributor

Trying this out now, looks promising.

A few requests:

  • let's change "setup:benchmarks" to "benchmarks:setup", and rename the jimbolla benchmark to stock-ticker.
  • Can you incorporate the table printing I added to the original perf benchmark repo? I did that last week, but forgot to push the commits until just now (see this commit specifically, which requires the cli-table2 package).
  • I'm not sure the / fpsValuesWithoutFirst.length || 0; line is safe as-is. If the array is empty, that would be a div-by-zero. Unlikely, but maybe push the 0 into the array if it's empty instead?
  • I think the benchmarks folder maybe needs its own package.json and have the cli-table and puppeteer dependencies there instead of in each benchmark itself.
  • For real extra credit, any way we can have some kind of command to check out and build specific versions of React-Redux automatically, instead of having them pre-built or something?

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2018

I don't think having it check out versions of react-redux is a good idea. What if we are trying to test a pull request? it gets unreasonably complex quite quickly, when the process for adding a new version is to check out your version, run build, and copy the react-redux.min.js over. Much easier to manage.

I'm not sure about the benchmarks folder package.json, it will probably work, but have to try it out.

The fpsValuesWithoutFirst.length || 0 is safe, I tested it locally.

The others I will do, no problem

@cellog
Copy link
Contributor Author

cellog commented Sep 3, 2018

ok, @markerikson check it now, thanks for the feedback

@markerikson
Copy link
Contributor

Updated it to print a table after each benchmark run, instead of waiting for all of them to finish.

@markerikson
Copy link
Contributor

Awright, let's get this in and move on.

@markerikson markerikson merged commit e915cfd into reduxjs:master Sep 3, 2018
timdorr added a commit that referenced this pull request Sep 4, 2018
@timdorr
Copy link
Member

timdorr commented Sep 4, 2018

Sorry, but I actually disagree with this merging in as-is. Not that we shouldn't have a benchmark or that @cellog hasn't done a good job, but I think it shouldn't go into this repo. Or at the very least, we shouldn't pack in other arbitrary versions of the library.

Right now this is purpose-built to compare a subset of versions against each other, which is our current concern. But that won't be a long-term issue and that's not what an in-repo benchmark should be IMHO. It should instead be to record the performance of the current version that is alongside it in the repo. That can be included with the test suite and tracked over time (something like http://isfiberreadyyet.com/) to know if we're getting better or when a regression was introduced.

If we want to check specific builds/versions, we should be able to do so. But those should be provided from artifacts stored outside of this repo (most likely on your local machine). Alternatively, we could have a separate repo for versions of the library hooked up to this benchmarking tool (to prevent commit thrash here). Third option: Extract this tooling into a separate repo with a published package (@rduxjs/react-benchmark) that we can import here for the test suite.

Given my objection, I'm going to revert this back out for now (stay calm everyone) so we can discuss my objections further and make appropriate tweaks.

@timdorr timdorr mentioned this pull request Sep 4, 2018
timdorr added a commit that referenced this pull request Sep 4, 2018
@markerikson
Copy link
Contributor

Fair enough, and I was having some of the same mental debates myself. Further suggestions?

@timdorr
Copy link
Member

timdorr commented Sep 4, 2018

Do we want to just set this up in a @reduxjs org repo for now? I really like the idea of making this available as a published package, as we could also theoretically use that for Redux itself. It's certainly a lot of work, so a separate repo is a good first baby step in that direction.

If maintaining artifact versions of the library to test against is a long-term need, we could have those stored in a separate branch in git to avoid commit thrash on the master branch. Then just hook the tooling up to that to make accessing it easier.

@markerikson
Copy link
Contributor

Yeah, a separate repo would be my inclination too.

I'm not sure that committing the artifacts was necessarily going to be the final approach we used, it was just what we had at the time.

I do rather like the looks of the speedracer tool that was linked over in #983 , it just doesn't seem like it has all the features we want right now, and hasn't been worked on in a while.

@timdorr
Copy link
Member

timdorr commented Sep 4, 2018

We could fork it and make it great again.

@markerikson
Copy link
Contributor

Heh. I'm entirely interested in potentially using it, but for the moment we've got a setup that works, and I'd rather focus on getting another 2-3 benchmark apps set up.

You want to throw together a new repo? My brain's kinda frying atm and I'm probably done doing work-ish stuff for the evening.

@cellog
Copy link
Contributor Author

cellog commented Sep 4, 2018

Any chance I could have commit access to that new repo? I'm happy to do PR land if not. I don't want commit access to react-redux, to remove any possibility of ambiguity.

@markerikson
Copy link
Contributor

Done. Created https://github.com/reduxjs/react-redux-benchmarks , added you with commit access.

I'm about to head to bed, but I'd say let's just copy over the contents of the benchmarks folder to the root of this repo and go from there.

@cellog
Copy link
Contributor Author

cellog commented Sep 4, 2018

I'll wait until at least tomorrow before I start any work, could we open an issue there to continue discussion?

@markerikson
Copy link
Contributor

Done.

@timdorr
Copy link
Member

timdorr commented Sep 4, 2018

Go nuts in there. If you need anything with Travis set up or other tools, let me know.

albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
* initial work on setting up a benchmark suite

it works EXCEPT building fails and I can't figure out why. something wonky in react-app-rewired

* fix setup script

* add benchmarks script

* remove unused imports

* fix linting, categories being undefined

also make the info output look nicer

* use colon instead of pipe as separator

* show 0 when fps is too slow for school

* add explanation of what good is

* jimbolla->stockticker, setup:benchmarks->benchmarks:setup

* lift puppeteer and tracealyzer into benchmarks, jimbolla->stockticker in package.json

* use cli-table2 for pretty results

* Print results table after each benchmark run

* Move express and recursive-copy deps to benchmarks folder
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants