connect creates new Props map object on every Action dispatch, which must be deeply compared #434
Comments
dave-doty
changed the title
|
I think this may be related to the function In Chrome Dev tools, I set a breakpoint at the equals method of one of my "way down at the bottom of the tree" objects, an object that should not be getting compared on an On many of those calls, the stack trace looks like this: (rest cut off, but I can post it here if it would be helpful) The interesting part is lines 208 and 209: bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) =>
areOwnPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));If I go to that stack frame and check out some variables in the console, here's what I find: In other words, I tried changing lines 206 - 215 to bool handleAreStatesEqual(ReactInteropValue jsNext, ReactInteropValue jsPrev) =>
identical(jsNext, jsPrev)? true: areStatesEqual(unwrapInteropValue(jsNext), unwrapInteropValue(jsPrev));
bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) =>
identical(jsNext, jsPrev)? true: areOwnPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));
bool handleAreStatePropsEqual(JsMap jsNext, JsMap jsPrev) =>
identical(jsNext, jsPrev)? true: areStatePropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));
bool handleAreMergedPropsEqual(JsMap jsNext, JsMap jsPrev) =>
identical(jsNext, jsPrev)? true: areMergedPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev));Unfortunately, it didn't speed things up all that much (but it did cut the time significantly, maybe by 30%). I'm still not sure what is the remaining bottleneck. |
|
Hey @dave-doty, sorry to hear you're hitting performance issues! One thing I'd recommend when doing perf profiling is to use a dart2js build, which can be done by passing That being said, it would still be nice to optimize things for the DDC so that you have a good experience when developing your app.
If you're seeing that comparison within a _shallowMapEquality from one of Nice find on that extra bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) {
final next = jsPropsToTProps(jsNext);
// Optimize for when these are the same object; re-use the same typed map
final prev = identical(prev, next) ? next : jsPropsToTProps(jsPrev);
return areOwnPropsEqual(next, prev);
} |
|
@greglittlefield-wf Thanks for the advice!
My problem when profiling dart2js-compiled code is that the source references become a complete mystery to me. Everything is compiled into unreadable minified JS code, so I don't know how to tell what part of my Dart code is causing a slowdown. But the performance issues are definitely also a problem with dart2js compiled code. It's disappointing to me that Google discontinued supporting Dartium, because it was nice to have a completely Dart-aware profiler. There appears to still be such a profiler for Dart VM, but that's not an option for web code using the HTML package. Aside: for reasons that elude me, the dartdevc version of my application is faster than the dart2js release version. So in the interest of pure pragmatism, I've actually been trying to figure out a way to host the dartdevc version, but this seems nontrivial to implement. I get the errors "Failed to load resource: net::ERR_FILE_NOT_FOUND" in stack_trace_mapper.dart.js:1 and require.js:1, and the only advice I could find online for how to host dartdevc-compiled code in production is "don't do it". :) |
bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) {
final next = jsPropsToTProps(jsNext);
// Optimize for when these are the same object; re-use the same typed map
final prev = identical(prev, next) ? next : jsPropsToTProps(jsPrev);
return areOwnPropsEqual(next, prev);
}Should that say Is the call to |
Having read up on React/Redux a bit more, I think I understand better what is happening. It is not merely that a single instance of my entire State tree is being passed to a deep equality check somewhere. (Actually I had this on one component near the top, and I replaced that My understanding is that on every I've read that the reselect library is helpful here when In summary, I suspect that the source of the slowdown is that I have a state tree with hundreds of objects, each of which is represented by a few (~2-15) view components, so hundreds-to-thousands of view components. Every one of these components is connected, and every one, on every Each node in my state tree ends up getting compared at some point; in fact, multiple times when multiple view components depend on the same state node. So it's actually worse than if I had just one single deep comparison of my entire State tree on each In the regime where all non-rendering code is essentially instant, I understand the advice to connect many view components. However, I suspect that my case is different, and it may be better for me to do a style more like React without Redux, where Redux connects only a few components near the top, and the rest of the React tree is unaware of Redux, passing down props as needed. I don't know for sure, and it takes several days of refactoring to test out such ideas, which is why I'm asking around to see if there's something else I possibly missed first. But I suspect that I'll get better performance if I do the following: whenever a view component appears only once (e.g. |
For that, you'll need to compile dart2js unminified. In your build.yaml: targets:
$default:
builders:
build_web_compilers|entrypoint:
# These are globs for the entrypoints you want to compile.
generate_for:
- web/**.dart
options:
compiler: dart2js
dart2js_args:
- --no-minifyThen, all the JS methods should have similar names to their Dart counterparts
Hm, one potential cause of that could be JS interop; dartdevc has faster JS interop than dart2js in some cases.
It should be constant time. You could definitely try that change locally if you want, though.
One thing that memoized selector will give you is that they will return identical props maps if the inputs don't change, resulting in faster props equality checks.
Another thing you can look into is setting
That's a pretty substantial amount of view components; I'm not an expert on Redux, but it's possible it doesn't scale to those kinds of numbers. I'd check https://redux.js.org/faq/performance and make sure you're doing everything the way they say to be doing. In any case, I think I'd need to see a performance profile using un-minified dart2js to help further, since there's not too much more information to go off of. |
|
@greglittlefield-wf Thanks for all the help. :) I made Chrome Profiler profiles with both dartdevc-compiled and dart2js-compiled builds (without minification). profiles.zip I also included screenshots in the profiles. In each case, you can see some squares with gray sides between them, and a circle on the left side. The mouse moving over a square should update the circle to have some arrows in it. In each case, I moved the pointer to the leftmost square, then right one, then right one again, and in the screenshots you can see how this updates the arrows within the circle. As you can see in the "Main" tab of the profile, a top-level "Event: mousemove" causes all the slowness in each case. It's about 180-200 ms per event in the dart2js version, and about 140 ms in the dartdevc version. In each case, I am gunning for sub-16 ms to get 60 FPS. The thing that made me suspect slow |
I'm sorry, I've been trying to wrap my head around how to do this, but I don't really understand. Here is a typical UiFactory<_$ViewProps> ConnectedView = connect<AppState, ViewProps>(
mapStateToPropsWithOwnProps: (state, props) {
Item item = state.items[props.itemId];
bool marked = state.isMarked(item);
var derivedProps = View()
..marked = marked
..item = item;
return derivedProps;
},
)(View);I'm not sure how to use reselect here. If I make selectors for the two derived props UiFactory<_$ViewProps> ConnectedView = connect<AppState, ViewProps>(
mapStateToPropsWithOwnProps: (state, props) {
Item item = itemSelector(state, props.itemId);
bool marked = markedSelector(state, item);
var derivedProps = View()
..marked = marked
..item = item;
return derivedProps;
},
)(View);It's not clear to me how that would help create an identical props map. A brand new object of type Perhaps you mean that I should pass the computed final identity = createSelector1((_) => _, (_) => _);
UiFactory<_$ViewProps> ConnectedView = connect<AppState, ViewProps>(
mapStateToPropsWithOwnProps: (state, props) {
Item item = state.items[props.itemId];
bool marked = state.isMarked(item);
var derivedProps = View()
..marked = marked
..item = item;
return identity(derivedProps);
},
)(View);Is the latter implementation close to what you meant? Update So I'm back to square one: how do you envision using reselect to avoid creating new Props objects when their constituent fields haven't changed in value? |
|
@greglittlefield-wf I tried my idea of ditching all the connected components far down in the view tree, and making only a few connected components near the top. Essentially I don't Then, using the The results are dramatic. My stall time per Action dispatched dropped from ~200 ms down to 15-20 ms and vastly reduced the jank I was seeing for frequently-dispatched Actions (i.e., 60 dispatch/second stuff, like responding to The annoying part is that now I need to pass all sorts of props down through the view tree so they can get from the big connected component at the top down to the many unconnected components. I know that even without React-Redux and the As I said, this could simply be due to me misunderstanding the proper and efficient way (possibly using the reselect package) to use React-Redux |
dave-doty
changed the title
|
Hey @dave-doty, so sorry for the delayed response; I lost track of this issue for a while. Thank you for putting together that perf data! You're right, most of it is in hashCode/equality, which points to those pesky Dart maps being the issue 😄. I have a draft PR up with some of the performance optimizations around mapStateToProps/ I'm curious to see if that would solve most of your perf issues! |
|
When I get more time later, I could dig up an old commit that uses lots of connected components to see how it compares. As I mentioned, in my most recent version, I'm using very few connected components, so |
|
@dave-doty - have a look at the changes in 3.7.0 that were just published and let us know if the perf issues you were seeing have been resolved! Also, 3.7.0 includes a stable implementation of |
|
@aaronlademann-wf Thanks! It's a bit difficult for me to test that right now. My most recent commit with many connected components was a previous version of OverReact, before UiComponent2 and so forth, so it doesn't run under the newest version. I am still curious to know a bit more about the plumping under the hood for React props. I do know that for the most part, things aren't re-rendering unnecessarily, but I'm still worried that a lot of time is being spent doing Props maps comparisons, in order to determine that no re-render is needed. But I think I've got some of my own performance issues to dig through first. But I'm also seeing (compared to my own hand-rolled PureComponent I was using before), some things now appear to be re-rendering unnecessarily. I'm profiling to see why, and I'm confused by this function in lib/src/util/equality.dart (called by bool propsOrStateMapsEqual(Map a, Map b) {
if (identical(a, b)) return true;
if (a.length != b.length) return false;
for (final key in a.keys) {
if (!b.containsKey(key)) return false;
final bVal = b[key];
final aVal = a[key];
// Function tear-offs are not canonicalized so we have to do a simple
// equality check on them instead of checking identity.
// See: <https://github.com/dart-lang/sdk/issues/31665#issuecomment-352678783>
if (!identical(bVal, aVal)) {
if (!(bVal is Function && aVal is Function && bVal == aVal)) {
return false;
}
}
}
return true;
}particularly the bottom if statement: I have a prop (it's an instance of So my list prop is the same (it's not |
|
My own implementation of PureComponent was this (I think I copied it from something I found in one of your repos several months ago): import 'package:collection/collection.dart';
import 'package:react/react.dart';
/// Top-level ReactJS [PureComponent class](https://reactjs.org/docs/react-api.html#reactpurecomponent)
mixin PureComponent implements Component2 {
@override
bool shouldComponentUpdate(Map nextProps, Map nextState) {
return !PureComponent._shallowPropsEqual(props, nextProps) ||
!PureComponent._shallowStateEqual(state, nextState);
}
static bool _shallowPropsEqual(Map map1, Map map2) {
// Does this work, or does props.children always make this return false?
return MapEquality().equals(
Map.of(map1)..remove('key')..remove('ref')..remove('children'),
Map.of(map2)..remove('key')..remove('ref')..remove('children'),
) &&
ListEquality().equals(map1['children'], map2['children']);
}
static bool _shallowStateEqual(Map map1, Map map2) {
return MapEquality().equals(map1, map2);
}
} |
|
@dave-doty the implementation of shouldComponentUpdate is designed to mimic how React.PureComponent works - which uses strict equality/identity. I'm not very familiar with BuiltValue, but if a new object is created each time - it will fail the identity check. @greglittlefield-wf knows more about BuiltValue than I do - so he may have more thoughts there. It's possible we need more special cases in that function to be optimized for stuff like Built objects. The reason Function is special cased in that function is because function tear offs do not retain their identity when referenced outside their original closure - which means that tear off functions as prop values would cause re-renders every time if we used strict equality. |
|
Yes, built_value generally creates a new object. The general pattern for reducers is something like this: MyState reducer(MyState state, action) {
return state.rebuild((b) => b
..field1 = reducer_field1(field1, action)
..field2 = reducer_field2(field2, action)
);
}where the value Supposing that your action doesn't actually trigger But the way built_value is implemented, any access at all to any of a builder's fields (even just reading them, but of course above we're actually writing them) causes a new built object to be generated. So in general, every Action dispatch causes brand-new built objects to be allocated. I was worried this would slow things down, but after profiling, it's quite fast. Even when I have a huge state with thousands of objects, small updates take a few dozen microseconds, as do deep equality comparisons on built objects. I've spent some time on exploring the possibility of optimizing this and only creating a new object on But given how built_value is currently architected, it appears unlikely to be implemented. (In particular, it allows storing mutable objects like Lists, which means that even just reading them could result in mutation to them, i.e., such a field would be Do you know if there is a reason that ReactJS works the way it does? Is it unsafe to return In the meantime, I wonder if this is solvable with an annotation or some way for the client to specify an option to use I picked built_value because it seemed to be the go-to way to make immutable objects in Dart. Do you generally just recommend hand-rolling custom immutable objects for use with OverReact? |
…onentMixin in OverReact uses only identical, not ==, so doesn't work for us see this discussion: Workiva/over_react#434
|
I've done a bit of profiling of my performance issues. In my previous comment from Dec 28, 2019, I indicated that I was able to dramatically improve performance by using many fewer connected components further up the view tree, and using props drilling to pass props needed by nodes farther down the tree. Using my own I've tried to get to the bottom of why I'm seeing unnecessary re-renders, but I've found it quite difficult to trace through what exactly gets called. Here's a screenshot of a React DevTools profiler, where I have a list of components (each of type
But as you can see, they are all being re-rendered (though only to one level, unlike the one that actually changes). Here's the React DevTools-cited reason for re-rendering one of them that didn't change:
I've inspected and I know that those props are final empty_set_ends = BuiltSet<DNAEnd>();
// ...
if (selected_ends_in_strand.isEmpty) {
selected_ends_in_strand = empty_set_ends;
}
elts.add((DesignMainStrand()
..selected_ends_in_strand = selected_ends_in_strand
// ...Then React DevTools no longer reports the prop
So it appears that whatever is causing the re-render, can be avoided by having But I'm having a hard time tracking down exactly which line of code is causing React DevTools to report that those props are the reason for the change. (i.e., where is the comparison being done?) I've checked my Unfortunately, since built_value and built_collection fairly liberally create new objects (even when they are |
|
@dave-doty sorry for my delayed reply, and thank you for all the detail around your issues!
Greg and/or I will keep you posted. |
|
@aaronlademann-wf @greglittlefield-wf
Thanks, I look forward to hearing about any updates!
Yes, I've noticed that as well. (Though I assumed part of it was due to OverReact components, which also do compile-time code generation if I'm not mistaken.) I wonder if it's worth consulting with the built_value author David Morgan regarding ways to optimize built_value as well. I had a fairly extensive discussion about a possible heuristic to reduce the number of cases when calling Currently it works like this: say you have a built object My idea is instead to maintain the I surmised that this would reduce the number of new Built objects for this common use case of Redux reducers ( State reducer(State state, action) =>
state.rebuild((b) => b
..field1 = reducer_field1(state.field1, action)
..field2 = reducer_field2(state.field2, action)
..nested_builder_field3.replace(reducer_field3(state.nested_builder_field3, action))
);where if I implemented that idea. The results were not as dramatic as I had hoped, and also they seemed to generate new Built objects in more cases than I expected, but in one test on a large model and view, it reduced a re-render time from ≈ 700 ms to ≈ 400 ms. But, I'm hesitant to implement this in my app, since my ideas were uninformed about two things:
|
|
@dave-doty the over_react builder is actually quite fast on its own... the built_value one utilizes the |
|
@dave-doty I talked things over with @greglittlefield-wf and here were the takeaways:
|
Most of my components are not connected. Earlier on I tested and found it much faster to have only a few connected components near the top of the view tree, in spite of the official React-Redux advice to have many connected components. My profiling showed that in that case, a huge amount of time was being spent in the |
I think that the performance of the
connectfunction in OverReact redux, specifically the performance of the functions it sets up such ashandleAreOwnPropsEqual,handleAreStatePropsEqual, etc. is adversely affected by potentially unnecessarily equality testing.I can't set up a simple example to show the effect. It's just something I notice in my complex app with fairly large
State. I'm trying to do very frequent updates (drawing a box created by the user click-dragging), 60 times per second. It can barely render more than once or twice a second when the State is large, even though it is only updating one tiny little object in the State tree representing the dimensions of the box.I've done the recommended React/Redux optimizations such as setting up
connecton components that would take a while to render, and writingshouldComponentUpdateon unconnected components. I've profiled with React DevTools and I know that no components are re-rendering that don't need to, so I know it is not unnecessary re-renders making it slow. According to React DevTools, the total render time for everything is very fast (about 1 ms in each round up updating).The Chrome performance profiler is tough to pick apart, because it points to execution in dart_sdk.js, and who knows which Dart code caused that call.
But the profiler tells me that each round of updates takes at least 100 ms (preventing 60 Hz rendering), and 70% of this time is being spent in
_shallowMapEqualityin src\over_react_redux\over_react_redux.dart. I stepped through and found that (a representation of) my entire State tree is being deeply compared for equality using this function every time anyActionis dispatched.I was very careful to set up an immutable State using built_value and built_collection, only generating a new State (or new part of a subtree of the State) if something changes. I even re-wrote parts of built_value to cache
hashCodeso that it is not re-computed unnecessarily. I am also sure that my reducers, when they don't change any data, return the same object, so for such objects an equality comparison should be a one-line call toidentical(this, other).But somewhere in the depths of OverReact Redux, I think these carefully constructed objects are being translated into Js-backed Maps, where two different map objects exist that represent the same data, and they need to be deeply compared all the way down to the leaves, just to check that they are equal.
I could be wrong about this, because I don't really understand the architecture of OverReact Redux. But perhaps there is a way to do some optimizations within OverReact Redux so that when data in most of the State tree does not change, the same objects are kept in place so equality testing is a fast call to
identicaland does not need to recurse deeply.Alternately, do you have a suggestion for how to structure my code to prevent such unnecessary equality checking?
The text was updated successfully, but these errors were encountered: