Potential performance issues with using forwardRef #13456
Comments
|
I'll paste in the comments between @cellog and myself from Discord as we noticed this behavior on Saturday: Pasting some comments between myself and Greg on Discord:
|
|
to clarify, after disabling forwardRef unless explicitly asked for, the FPS on 1000 doubled, so it was a significant performance increase. This only happened when running large numbers of "stocks." At 500 stocks, the performance of all 3 (5.0.7, and the 2 patches) was 60 fps. The performance hits occurred at 2500-3500 "stocks." I should also note that it showed up not just in the fps measure, but in chrome traces (also generated by the benchmark app in a separate run). The turnaround time for processing state updates was double the length in milliseconds when passed through forwardRef. Again for clarity it's important to note that no refs were actually passed into the benchmark app, so this isn't specific to ref handling, but to forwardRef when no refs are passed in. |
|
Is there a benchmark we can use that reproduces the problem with just React (no React Redux)? |
|
Sure, the redux part of the benchmark is minimal, just needs to be stripped out. Just need to do it, will post when it's ready |
|
https://github.com/cellog/benchmark-react-redux-perf/tree/remove-redux is good to go, you can run it with env variable FORWARD set to 1 to enable forwardRef, leave it out to disable it. I haven't figured out how to modify the benchmark Because this version is basically redux 4, it's quite slow, so I changed number of components (in |
|
What should I be measuring? If I just record traces from the production build, I see this:
versus this:
Is this the kind of difference you're referring to? |
|
In our case, we were measuring overall FPS while the app was running. The Here's an example of the kinds of numbers we were seeing recently, copied and pasted from issue 1000:
In general, what we're seeing is that wrapping our components in I know it's not the best possible benchmark, but it's all I've managed to put together so far. |
|
How does it compare to wrapping in a functional component instead of forwardRef? |
|
Okay, I've got some benchmark numbers to add. The builds, in order:
I'm attaching a zip file with the captured Chrome perf traces for reference. (Note that per the benchmark repo, the "Avg FPS" and the Chrome trace are captured on separate runs for each build, so that the trace capture doesn't slow down the FPS.) react issue 13456 - React-Redux perf benchmark traces.zip Per the 995 branch results, it looks like both using |
|
I think it's worth looking into how worse the results are if you add more component layers (e.g. wrap in extra N In other words I'm arguing that if adding a |
|
Yep, that's the weakness with only having one benchmark app so far that only measures one primary metric. I'd like to get us set up with several other benchmark apps as well that would run some more scenarios. |
|
Yes, but part of what we are trying to measure is how heavy react-redux is in a stressful environment, like a stock ticker app, so little things are naturally amplified. This benchmark was helpful in finding several unnecessary slowdowns, and also proved decisively that using However, it does highlight that forwardRef is expensive in this situation. What I think is useful from a React perspective is the fact that forwardRef is also expensive in our benchmark even if no refs are present, which presents an opportunity to look at whether that is a necessary expense. I concluded that it was better to avoid forwardRef unless the user explicitly asks for it (which is similar to how react-redux works now anyways) based on what we learned. If you conclude "yes, the risk of expense is worth it" then the issue can be closed. Or perhaps mark it as a documentation issue if you want to note for library authors that forwardRef does have some expense in certain environments with lots of components updating rapidly. |
I think what Dan is trying to tease out, tho is whether forwardRef is the the expensive thing, or is the slow down the result of having more components. If it's forwardRef's fault there may be something to optimize, if it's the latter tho, the question may be, is the app complex enough to approximate a complex react app. e.g. if you replaced every forwardRef with an extra HoC or |
|
Per the benchmarks I pasted a bit earlier, it looks like |
|
Right, if you assume that forwardRef should be as expensive as a component with all the life cycle weight, then yes. |
|
Not with lifecycle weight, but I'd expect it to be roughly equivalent to a functional component. Because in a way it is one — you do get a render function there. Which also means that if you don't use lifecycles, you can move the rendering logic into the forwardRef callback, thus removing the layer inside of it. Then you'd go back to one layer. |
|
If it's not equivalent to functional component it would be interesting to profile what makes it different. It's not obvious to me. |
|
Yeah, problem is that I can try re-running the benchmark a couple times. There's obviously not a lot of difference between wrapping in a |
|
What does |
|
Hmm. So in my PR 995, I'm using an outer/inner component approach and doing the bulk of the work in the inner component's In Greg's PR 1000... huh. Okay, also only binding a couple methods in the constructor: PR 1000 In both cases, a class method is being given as the render prop for the context consumer, and it's being bound so it can safely access So, not actually a lot, but I don't immediately see a way to do with a functional component. |
|
I see what you're suggesting. If there is a way to create a per-instance cache with a functional component, I would happily convert the top-level component to one, and use forwardRef on it, then benchmark that |
|
OK, I tried it. There's no way to create a per-instance cache outside of a class component (that I can figure) so that won't work. Basically, in order to prevent unnecessary re-render on an unimportant part of state updating, we have to use something that can save state, which a functional component can't. There are other ways to solve this with an API change for the future, but we are focusing on getting something out that legacy apps can use as a path to migrate to the new API without a complete rewrite |
|
When do you want to release that? We have something in the works that might help these cases but it’ll take a few months to get it out. |
|
Depends on when we get a more comprehensive benchmark suite up and running. Is there a PR to follow or is it still internal maybe-will-happen? |
|
It will definitely happen but I don't expect to definitely see a PR until the end of October. |
|
Ok :) probably best to pretend it doesn't exist until then unless you are allowed to give a hint how we can prepare the code to be ready for it. All very mysterious and exciting, are you all releasing a new iPhone? |
thank you |
|
Just wanted to call out that we ran into this issue - we're trying to get a sticky header to stick on scroll, but we're missing a frame or two which causes a noticeable flicker. A chunk of that seems to be because of forwardRef:
I'll be testing some ways of doing this outside of React but it seemed like there were some issues doing this the "React" way. Though, any time you're async rendering and you -have- to hit a certain frame timing you're sort of setting yourself up for pain. edit: decided to just ditch IE11 support and go for position:sticky so there isn't anything blocking us here, still figured I'd leave this as an observation on forwardRef perf. |
|
I'm mostly curious at to why the difference seems so large between development and production. I have a virtual table that renders a decent amount of items, even in a small window. In production, it's basically fine for me. In development, I see this:
And so on for ~50 items. As you can see forwardRef is most of the work here, which seems odd. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you! |
|
@natew My guess is it's the User Timing API itself being slow as hell. We've removed it on master because of how misleading it is. |
Do you want to request a feature or report a bug?
Bug (assuming that a potential drop in perf counts as a "bug")
What is the current behavior?
I've been setting up a rudimentary perf benchmark harness for React-Redux over in reduxjs/react-redux#983 , so that we can compare performance between builds of React-Redux as we work on version 6. The harness runs one or more benchmark apps in headless Puppeteer, running them once to capture FPS values and a second time to capture Chrome trace reports automatically.
Currently, there's only one benchmark app: a "stock ticker"-type stress test that measures FPS by using the
fps-emitterpackage (which relies onrequestAnimationFrame). It's rough, but it does show meaningful differences in FPS values as we swap between different React-Redux builds and change the number of connected components in the benchmark.We have two WIP candidate PRs for React-Redux v6: reduxjs/react-redux#995 and reduxjs/react-redux#1000 . Initial testing showed that PR 995 was almost as fast as our existing 5.0.7 version, while PR 995 was slower in some runs. However, my initial push of the 995 branch did not include use of
React.forwardRef, while the 1000 branch already had that added.I spent Saturday adding
forwardRefand a couple other pieces of functionality to the 995 branch, then re-ran the benchmarks. Per ourcomments in the 983 issue, we saw that the 995 branch had suddenly gotten slower, and that removingforwardReffrom the 1000 branch sped things up by a few FPS.This is admittedly a very artificial stress test scenario, but it's also likely that React-Redux apps can have hundreds or thousands of connected components at once, so the potential slowdown seems concerning.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:
The current perf benchmark repo is at https://github.com/markerikson/benchmark-react-redux-perf . It uses UMD builds of React-Redux so that it can easily switch which build is being used. Several UMD builds are already committed there in the root of the repo.
6.0-markis the 995 branch, and6.0-gregis the 1000 branch. They can be reproduced by building their respective branches from the PRs. I've been hand-copying the UMD build artifact from my React-Redux clone over to this benchmark folder and renaming it to whatever seems appropriate.Clone the repo,
yarnto install,yarn perfto build and run the benchmark. Number of connected components can be modified insrc/constants.js(requires rebuilding). The harness determines which UMD build versions to run based on an array inperfBenchmark.js, around line 11 (const VERSIONS = ["5.0.7", "6.0-mark"]).What is the expected behavior?
That there would be minimal to no impact in perf when
forwardRefis used to wrap a component such as a HOC, and that HOC is used widely in an application.Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
I've been testing this with Puppeteer 1.6.2, on Windows 10. It looks like the locally installed version of Chromium is 69.0.3494.0 .
Current version of React is 16.4.2, UMD build (as React-Redux UMD expects that both React and Redux are also global variables). I have not tested against a prior version of React, as we are specifically targeting 16.4+ with this next version of React-Redux.
The text was updated successfully, but these errors were encountered: