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

Add performance benchmark suite #983

Closed
markerikson opened this issue Jul 25, 2018 · 22 comments
Closed

Add performance benchmark suite #983

markerikson opened this issue Jul 25, 2018 · 22 comments

Comments

@markerikson
Copy link
Contributor

markerikson commented Jul 25, 2018

Update: See this comment for current status and requests for help

Prior and related references: #104 , #398 , #407, #416 , #898 .

I was doing a bit of searching earlier for any links to benchmarks I could find in the issues, and here's the links I found:

https://github.com/jimbolla/react-redux-perf
https://github.com/mweststrate/redux-todomvc
https://github.com/jscriptcoder/stressing-redux
https://github.com/broadsw0rd/react-redux-perf
https://github.com/somebody32/high-performance-redux/tree/gh-pages/assets
https://github.com/coderitual/redux-performance
#686 (comment)
https://github.com/Kalkut/redux-data-frequency-benchmark

I think it would be very valuable if we were to swipe some of these somehow and set up some performance benchmarks. This would be particularly valuable as we evaluate the changes in 5.1 (per #980 ), and the notional future 6.0 and 7.0 releases as laid out in #950 .

@timdorr
Copy link
Member

timdorr commented Jul 26, 2018

Interestingly, I'm working on a startup (though I haven't given it much attention lately) to provide this kind of tracking of metrics over time. Maybe I should get my ass in gear so we can hook this up and track progress 😄

@markerikson
Copy link
Contributor Author

markerikson commented Aug 8, 2018

I've set up an initial standalone perf benchmark experiment repo here:

https://github.com/markerikson/benchmark-react-redux-perf

It builds a CRA version of Jim Bolla's stock ticker benchmark, which has been configured to log FPS changes as performance API events. I can retrieve those from Puppeteer and log them out afterwards.

I have the CRA project modified to define React, Redux, and React-Redux as external libs in a prod build. That lets me easily swap out the React-Redux version by overwriting a script tag and reloading the page (and is sorta similar to what React does in their benchmark setup).

Jim's benchmark is a stock ticker app, and originally had 330 connected stock listing components. I've bumped that up to 1000 for stress testing, and run the test with four versions:

I haven't done any investigation into why #980 is slower.

My next step is to set up a way to build and run multiple benchmark apps with varying versions of React-Redux and capture the results. Once I've got that figured out, I can move this setup into the React-Redux repo and automate things, then we can start adding more benchmark apps.

After that, we ought to look at instrumenting the codebase for perf timings, probably with a separate "profiling" build or something. I'd be interested in things like how long it takes to run mapState, how long it takes from the start of notifying subscribers to the end, etc.

@markerikson
Copy link
Contributor Author

markerikson commented Aug 22, 2018

Pasting some comments between myself and Greg on Discord:

[8:07 PM] acemarke:oh, wow
[8:08 PM] acemarke: the commits I just pushed are way slower than the prior version
[8:08 PM] acemarke: I think a lot of this has to do with the forwardRef and stuff
[8:08 PM] acemarke: dropped from 24-26 to 17-19
[8:08 PM] acemarke: at 2500 components
[8:08 PM] acemarke: could also be that my implementation isn't very good

[8:10 PM] CelloG:dammit :/
[8:10 PM] CelloG: I'll try removing forwardRef and see what happens
[8:11 PM] CelloG: if that speeds it up we need to add back withRef
[8:20 PM] CelloG: you're right!

[8:27 PM] CelloG: should probably tell the React team
[8:27 PM] CelloG: that's a huge issue, it should be performance-agnostic if it isn't passing an actual ref
[8:48 PM] CelloG: Ok, running the latest version now. This only uses forwardRef if withRef is set to the text "forwardRef" so we can catch folks using withRef set to true and warn them to not rely on getWrappedInstance since it don't exist no more
[8:49 PM] CelloG: And my implementation is now doing better. Let me see how it matches to 5.0.7
[8:51 PM] acemarke: Yeah, I was thinking we'd introduce a new forwardRef option or something
[8:54 PM] CelloG: ok, I just pushed the commit
[9:01 PM] CelloG: this version I get slightly faster than your old one, but still 2 fps slower than 5.0.7
[9:01 PM] CelloG: but we are in the ballpark. I'm going to re-run and see if there is any change
[8:20 PM] CelloG: after disabling forwardRef, mine is faster
[8:20 PM] CelloG: 2 fps
[8:21 PM] CelloG: than 995

@timdorr
Copy link
Member

timdorr commented Aug 22, 2018

Is there an issue open on the React repo?

@markerikson
Copy link
Contributor Author

We haven't opened one yet, and I didn't see one when I was looking earlier this evening.

The chat conversation is from Saturday. I just pinged Dan on Twitter, and he said he might be able to take a look.

@markerikson
Copy link
Contributor Author

Aaaand opened up React issue 13456 referencing this issue.

@sebinsua
Copy link

@markerikson I didn't see speedracer in your list of links. :)

@markerikson
Copy link
Contributor Author

@sebinsua : oh, wow! That looks like exactly what I was sorta trying to build myself :) The biggest difference I immediately see is that my setup will re-run the same benchmark multiple times for multiple versions of React-Redux.

I'm currently using Puppeteer for the headless Chrome - can speedracer use that?

I'd really appreciate any advice you can offer on using speedracer for this scenario.

@markerikson
Copy link
Contributor Author

markerikson commented Aug 25, 2018

Okay, so for anyone visiting this thread who is interested in helping out, here's where things stand at the moment:

  • We currently have a passable start on a benchmark harness at https://github.com/markerikson/benchmark-react-redux-perf
  • That harness only has one app it can run (a "stock ticker"-type scenario), and really only captures one primary metric (Frames Per Second)
  • The harness is in a separate repo, and not part of the React-Redux repo
  • The harness currently prints out FPS and Chrome perf trace values for each version of React-Redux it tests against

What I really want is:

  • Multiple benchmark apps that represent different scenarios ("a shallow page full of single connected components", "a deep tree with connected components in different places", "an app that actually changes the UI structure over time")
  • Hopefully some more metrics captured
  • Ability to easily run all this against multiple builds/versions of React-Redux
  • Automatic +- change comparisons between versions
  • All this integrated into the React-Redux repo as a sub-folder called benchmarks, similar to our existing tests folder

As an immediate goal, I'd appreciate assistance in building out some more benchmark apps, based on both the list of examples I linked at the start of the issue, as well as any other good ideas people have for meaningful example apps.

If you're interested in helping out, please comment in this thread, and we can coordinate from there.

Thanks!

@chirgjn
Copy link

chirgjn commented Aug 26, 2018

I'll start with the single shallow page one, first thing tonight.

Do I need to setup anything or I can just make a react app with CRA?

@markerikson
Copy link
Contributor Author

@chirgjn : I think we actually have the "single shallow page" aspect covered already with the existing "stock ticker" app. I think it would be better if we can come up with a few other app ideas for the other scenarios, maybe based on some of the other benchmark repos I linked in the first comment in the thread.

For now, go ahead and fork https://github.com/markerikson/benchmark-react-redux-perf , which has the perf benchmark tooling set up. The application part of that has some code in there to capture Frames Per Second values and attach those to the window object, which the perf benchmark runner script can grab afterwards.

From there, you can rewrite the actual application part to be something different than the stock ticker example.

@zenobht
Copy link

zenobht commented Aug 27, 2018

@markerikson I can help. I will have a look at the links you provided.

@Avi98
Copy link

Avi98 commented Aug 28, 2018

@markerikson i can start with creating readme of all the important points from all the links.

@markerikson
Copy link
Contributor Author

@Avi98 : sounds good.

@mattridley
Copy link

@markerikson Happy to help out with building some more benchmark apps out. I had a start adapting react-todomvc (similarly to Michel's) into the benchmark framework but it quickly dawned on me that really it is just another single shallow page like the ticker example.

Do you have anything in mind for "a deep tree with connected components in different places" or "an app that actually changes the UI structure over time"? I am assuming we want to create apps that are realistic rather than simply creating a deep tree just for the sake of the benchmark?

@markerikson
Copy link
Contributor Author

@mattridley : no, I don't immediately have ideas for apps that fit those categories, although maybe there's something listed at https://github.com/markerikson/redux-ecosystem-links/blob/master/apps-and-examples.md that we could adapt or use as an example.

I can see points for both "realistic" apps and "unrealistic" stress tests. It would also help if we can come up with more metrics to measure besides the current FPS meter, but it's at least something objective for now.

Actually, perhaps the "tree-view" example in the Redux repo would be an acceptable starting point for a "deep tree" benchmark: https://github.com/reduxjs/redux/tree/master/examples/tree-view .

@markerikson
Copy link
Contributor Author

Okay, @cellog has ported the benchmark runner and the stockticker app over into the main repo, and that's been merged in.

@markerikson
Copy link
Contributor Author

I'm going to look into porting the tree-view example over, and @cellog is going to try to adapt the twitter-lite repro app.

@markerikson
Copy link
Contributor Author

@sebinsua : I just looked over the speedracer repo again. It sounds like a lot of the planned features are things I would want (like speedracer/speedracer#5 ), but the tool doesn't seem to have been updated in a while, and no indication that any of those roadmap items are being worked on.

@markerikson
Copy link
Contributor Author

Reversing course a bit - @timdorr feels the benchmarks shouldn't go in the repo directly, or at least not the way we were going, so he just backed out that commit. I'm okay with that.

I'm inclined to set this up as a separate repo instead.

@markerikson
Copy link
Contributor Author

markerikson commented Sep 5, 2018

We've got a new repo added at https://github.com/reduxjs/react-redux-benchmarks , and @cellog has ported over the existing benchmarks setup.

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

No branches or pull requests

7 participants