connect creates new Props map object on every Action dispatch, which must be deeply compared

@greglittlefield-wf

Issue Type: FEATURE REQUEST

over_react Version(s): 3.1.5

I think that the performance of the connect function in OverReact redux, specifically the performance of the functions it sets up such as handleAreOwnPropsEqual, 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 connect on components that would take a while to render, and writing shouldComponentUpdate on 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 _shallowMapEquality in 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 any Action is 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 hashCode so 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 to identical(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 identical and does not need to recurse deeply.

Alternately, do you have a suggestion for how to structure my code to prevent such unnecessary equality checking?

FYI: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @evanweible-wf @maxwellpeterson-wf

I think this may be related to the function jsPropsToTProps:

https://github.com/Workiva/over_react/blob/master/lib/src/over_react_redux/over_react_redux.dart#L155

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 Action dispatch because neither it nor its parents or grandparents changed as a result of that Action.

On many of those calls, the stack trace looks like this:

_equals (src\state\bound_substrand.g.dart:362)
equals (dart_sdk.js:4265)
equals (equality.dart:85)
_equals (equality.dart:284)
equals (dart_sdk.js:4265)
_get (dart_sdk.js:19715)
equals (equality.dart:318)
_shallowMapEquality (src\over_react_redux\over_react_redux.dart:245)
handleAreOwnPropsEqual (src\over_react_redux\over_react_redux.dart:209)  <<< focus on this
_checkAndCall (dart_sdk.js:4074)
dcall (dart_sdk.js:4079)
ret (dart_sdk.js:52079)
handleSubsequentCalls (selectorFactory.js:52)
pureFinalPropsSelector (selectorFactory.js:63)
checkForUpdates (connectAdvanced.js:245)
handleChangeWrapper (Subscription.js:70)
(anonymous) (Subscription.js:25)
batchedUpdates$1 (react-dom.development.js:4149)
...

(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:

> jsPrev
{DesignMainBoundSubstrandProps.substrand: b…d._…d.__, DesignMainBoundSubstrandProps.color: c…$.H…r.fromRgb, children: Array(1)}
DesignMainBoundSubstrandProps.color: color$.HexColor.fromRgb {Symbol(RgbColor.r): 0, Symbol(RgbColor.g): 102, Symbol(RgbColor.b): 204}
DesignMainBoundSubstrandProps.substrand: bound_substrand._$BoundSubstrand.__ {Symbol(__dnaend_start): d…d._…d.__, Symbol(__dnaend_end): d…d._…d.__, Symbol(_$BoundSubstrand.helix): 1, Symbol(_$BoundSubstrand.forward): false, Symbol(_$BoundSubstrand.start): 0, …}
children: [null, Symbol(_identityHashCode): 537510498]
Symbol(dartx.hashCode): (...)
Symbol(dartx.runtimeType): (...)
__proto__: Object
> jsPrev == jsNext
true
> jsPrev === jsNext
true
> areOwnPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev))
true
> jsPropsToTProps(jsNext) === jsPropsToTProps(jsPrev)
false

In other words, jsPrev and jsNext refer to the same object. That object is a Props map for an OverReact Component2 named DesignMainBoundSubstrand. Because the variables reference the same object, it should in principle be a one-line call to identical to see that they are equal, but instead they are converted to two different objects by jsPropsToTProps, which are then deeply compared by areOwnPropsEqual (which is implemented by MapEquality().equals()).

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.

@dave-doty

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 --release to webdev or build_runner. The Dart Dev Compiler is not optimized for performance, so it may be slower than dart2js in certain scenarios. This will allow you to gauge the performance as your end-users will experience it.

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.

I stepped through and found that (a representation of) my entire State tree is being deeply compared for equality using [_shallowMapEquality] every time any Action is dispatched.

If you're seeing that comparison within a _shallowMapEquality from one of areOwnPropsEqual, areStatePropsEqual, areMergedPropsEqual, is it possible that your entire state is being passed in as a prop somewhere? If so, you should map the pieces of state you need to props so that only those need to be compared instead of the entire state.

Nice find on that extra JsMap creation! I think we'd want a little bit different implementation, though, to allow users to return false even if the objects are identical (which isn't recommended, but may be necessary in some cases):

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

@greglittlefield-wf Thanks for the advice!

One thing I'd recommend when doing perf profiling is to use a dart2js build, which can be done by passing --release to webdev or build_runner.

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". :)

@greglittlefield-wf

@greglittlefield-wf

I think we'd want a little bit different implementation, though, to allow users to return false even if the objects are identical (which isn't recommended, but may be necessary in some cases):

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 identical(jsPrev, jsNext) instead of identical(prev, next)?

Is the call to jsPropsToTProps expensive? (i.e., more than constant time) If so, can I suggest an implementation based on allow the user to globally configure whether they want to allow returning false even when identical(jsPrev, jsNext) is true? That way one could ask the library to skip calling jsPropsToTProps entirely.

@greglittlefield-wf

@greglittlefield-wf

If you're seeing that comparison within a _shallowMapEquality from one of areOwnPropsEqual, areStatePropsEqual, areMergedPropsEqual, is it possible that your entire state is being passed in as a prop somewhere? If so, you should map the pieces of state you need to props so that only those need to be compared instead of the entire state.

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 connect with a smarter one extracting out only the props needed by children, but the performance issues remain.)

My understanding is that on every dispatch to the Redux store, every connected component has its mapStateToProps function called. I followed the advice given here to use many connected components throughout my view tree, using mapStateToProps to gather whatever information they need from the state.

I've read that the reselect library is helpful here when mapStateToProps needs to reshape data in a time-consuming way, but this is not the case for any of my mapStateToProps functions. All of them simply gather objects from the state tree and pass them along as props. This either involves directly accessing fields (e.g., ..prop1 = state.child.grandchild.field) or map lookups (e.g., ..prop2 = state.child.grandchild.someMap[prop1]). So it seems that reselect's technique of memoizing selector calls won't help here, because we'd simply replace one constant-time map lookup (on a map stored directly in the state tree) with another (on the map backing the memoized selector). The number of such lookups won't change, and I think it's that number of lookups that is causing the problem.

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 dispatch, has its mapStateToProps called, and the cost of doing all those lookups to hashCode (for accessing maps) and equals/== (for testing prop equality) in total takes a few hundred milliseconds.

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 dispatch.

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. ListOfAllItemsComponent), but it has underneath is a large list of child view components (e.g., each one is an ItemComponent), do not connect the children or their descendents. Instead, only connect the parent. As long as state updates don't affect the portion of the state the parent depends on, then we will only have to pay once to do the state comparison on the parent (since BuiltList and BuiltSet cache their hashCode's, this will be fast), and when that indicates no need to re-render, then none of the children will be required to do any comparison.

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.

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-minify

Then, all the JS methods should have similar names to their Dart counterparts

For reasons that elude me, the dartdevc version of my application is faster than the dart2js release version.

Hm, one potential cause of that could be JS interop; dartdevc has faster JS interop than dart2js in some cases.

Should that say identical(jsPrev, jsNext) instead of identical(prev, next)?

Is the call to jsPropsToTProps expensive? (i.e., more than constant time)
Yes, good catch; sorry about that.

Is the call to jsPropsToTProps expensive? (i.e., more than constant time) If so, can I suggest an implementation based on allow the user to globally configure whether they want to allow returning false even when identical(jsPrev, jsNext) is true? That way one could ask the library to skip calling jsPropsToTProps entirely.

It should be constant time. You could definitely try that change locally if you want, though.

So it seems that reselect's technique of memoizing selector calls won't help here

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.

, because we'd simply replace one constant-time map lookup (on a map stored directly in the state tree) with another (on the map backing the memoized selector).
I might be misinterpreting this statement, but FYI the memoize package doesn't use maps to check equality of arguments https://github.com/wrike/memoize_dart/blob/master/lib/memoize.dart#L23-L24

Another thing you can look into is setting areStatesEqual to only look at the part of the state you're interested in.

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.

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

@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 mapStateToProps calls is that, way down near the leaves of that flamegraph, it appears to be mostly calls to handleAreStatePropsEqual, which subsequently calls a lot of the equals and hashCode methods from various parts of my State.

@greglittlefield-wf

@greglittlefield-wf

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.

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 connect call:

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 selected and item, it might look like this:

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 ViewProps is still being created by the View() call. All the selectors did was make sure that the underlying fields of the ViewProps object are the same, but it's still a new ViewProps object.

Perhaps you mean that I should pass the computed props object itself (of type ViewProps, referenced by the variable derived_props in my example) to a selector that just computes the identity function, whose only purpose is to cache the ViewProps object and ensure that the same one is returned each time? i.e.,

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
After looking through the implementation of memo1 in the memoize package used by reselect, I see that this won't work. It only caches the most recently passed argument, so using the same identical function for all views won't work.

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

@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 connect anything that is one component in a big list of components of the same type.

Then, using the PureComponent implementation of @aaronlademann-wf here: Workiva/react-dart@ce372c4, the parent component that renders the big list of children doesn't render unless it needs to, and none of the PropMaps of its many children need to be compared.

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 onMouseMove).

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 connect function, there's something called "context" that's supposed to help avoid this: https://reactjs.org/docs/context.html, but I'm worried that its implementation will be similar to connect (in fact, my understanding is that connect uses React context behind the scenes) and will end up expensively creating a bunch of PropsMap objects and comparing them unnecessarily, just in order to verify that the many deep view components don't need to be re-rendered.

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 connect to wire up the AppState to the various connected components.

@dave-doty

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/are*PropsEqual, sidestepping Dart code by default and using the built-in equality checking in connect: #462

I'm curious to see if that would solve most of your perf issues!

@greglittlefield-wf

@greglittlefield-wf

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 connect isn't getting called much. Nonetheless, I did profiling before and after for the slowest-rendering stuff, and saw the same performance before and after this update. But since it's only calling connect a few times (instead of on hundreds of components) I think the bottleneck is now elsewhere.

@dave-doty

@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 PureComponentMixin, which should allow you to build React.PureComponentish OverReact components!

@aaronlademann-wf

@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 PureComponentMixin.shouldComponentUpdate):

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: if (!(bVal is Function && aVal is Function && bVal == aVal))

I have a prop (it's an instance of BuiltList from the built_collections package). It's equal from one render to the next as far as I'm concerned (it contains the same objects in the same order), but it's failing the is Function test. Here's the result of some print statements just before that if statement:

bVal is Function: false
aVal is Function: false
bVal == aVal: true

So my list prop is the same (it's not identical, but it represents the same list in the sense of having the same objects in the same order), so the component should not re-render, but it is re-rendering, because the prop is not a function. That doesn't make sense to me. However functions get handled, it seems that non-functions should be tested for equality using ==.

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

@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 b is a generated type called MyStateBuilder, which is essentially a mutable version of a MyState. In other words, the above is shorthand for "make a mutable builder from state, assign to its two fields, then build that into an immutable built object and return it".

Supposing that your action doesn't actually trigger reducer_field1 or reducer_field2 to do anything, they should return the same object (or at least some object that is == to their parameter).

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 rebuild() when actually necessary:

google/built_value.dart#774

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 identical to the old one but not == to it, which is why even reading a field causes build to create a brand new built object.)

Do you know if there is a reason that ReactJS works the way it does? Is it unsafe to return true from propsOrStateMapsEqual if bVal == aVal?

In the meantime, I wonder if this is solvable with an annotation or some way for the client to specify an option to use == in PureComponentMixin.shouldComponentUpdate, or simply a whole second mixin PureComponentDeepEqualityMixin that does the == comparison.

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?

switched back to custom implementation of PureComponent; new PureComp…

@aaronlademann-wf

@aaronlademann-wf

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 PureComponent implementation (here: #434 (comment)) keeps some unnecessary re-renders from happening, but not all.

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 DesignMainStrand), under a container component DesignMainStrands. The action being shown here changes one of the components (really, removes it and adds a new one; that's the one where the green part of the flamegraph goes a couple levels lower than the rest) and leaves the rest alone:

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 == to the previous props, but they are not identical. In fact most of them are empty sets. Indeed, if I pick one of them selected_ends_in_strand, and in the parent component DesignMainStrands, check explicitly to see if it is empty, and if so, set it equal to a fixed empty set:

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 selected_ends_in_strand as a reason to re-render:

So it appears that whatever is causing the re-render, can be avoided by having identical props as before (but not == props).

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 PureComponent implementation, and its shouldComponentUpdate method is returning false for every one of these components. But they are re-rendering anyway.

Unfortunately, since built_value and built_collection fairly liberally create new objects (even when they are == to the previous object from which they were built), I don't have much of a way to work around this unless built_value changes to be more conservative about creating new objects.

@dave-doty

@dave-doty sorry for my delayed reply, and thank you for all the detail around your issues!

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?

built_value is used pretty extensively in our organization (Workiva) - in enormous application(s) - so I would say you've made a sound choice. Its builder can begin to slow down build times precipitously at scale as a result of its reliance on the source_gen builder (something we plan on looking into at some point) - but for enforcing immutability, its the way to go. Since our teams use it internally - and they use OverReact alongside it extensively - we are motivated to identify ways to optimize for built_value use cases.

Greg and/or I will keep you posted.

@aaronlademann-wf

@aaronlademann-wf @greglittlefield-wf

Greg and/or I will keep you posted.

Thanks, I look forward to hearing about any updates!

Its builder can begin to slow down build times precipitously at scale as a result of its reliance on the source_gen builder

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 my_built_value.rebuild(...) ends up unnecessarily generating a new Built object even though the underlying data didn't change.

Currently it works like this: say you have a built object val and call var b = val.toBuilder(). b maintains a reference to val (a field called _$v in the generated source), and if any of its fields are accessed, even just to read, or to assign a field with the same value it had before, _$v is set to null (see here, or inspect the generated source of any Built class, e.g., this example). When calling build(), if _$v==null, a new Built object is generated.

My idea is instead to maintain the _$v reference until the next call to build(), and at that point, to referentially compare the fields of b._$v with those of b using identical(b.field1, b._$v.field1). (When the field is a nested builder, you first build it, and then compare that referentially to the corresponding field in b._$v.) If all fields are referentially identical, then simply return b._$v from b.build(). (The details are more complex than that, but that's the big picture.)

I surmised that this would reduce the number of new Built objects for this common use case of Redux reducers (rebuild is a convenience method for, "call toBuilder(), apply these changes to that Builder, and then call build() on that Builder"):

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 action is simply ignored by all three subreducers (most sub-reducers do indeed ignore most actions), then they will return their first argument unchanged, which means all three fields will pass the identical test (this is inductively true for nested_builder_field3, which is actually of type Builder, not Built).

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:

The internal details of how OverReact and its React-Redux bindings represent React props.
What ways are "safe" to modify built_value and built_collection while maintaining their guarantees of immutability and correctness.

@dave-doty

@dave-doty the over_react builder is actually quite fast on its own... the built_value one utilizes the source_gen builder - which relies on fully resolved AST - which is incredibly slow.

@dave-doty

@dave-doty I talked things over with @greglittlefield-wf and here were the takeaways:

In general - since you're using connect, the use of areStatesEqual would make more sense than overriding shouldComponentUpdate (either manually - or through the use of a mixin like PureComponentMixin). That way you can use == to compare your Built* values.
If you are still having issues where those built values are not evaluating as equal - but you're sure that they are - you could implement some memoization for those values.

since you're using connect, the use of areStatesEqual would make more sense than overriding shouldComponentUpdate (either manually - or through the use of a mixin like PureComponentMixin).

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 mapStateToProps functions of lower-level components, whose counts are in the hundreds or thousands. So in this case, I don't even think it was re-rendering that was slowing things down.

Navigation Menu

Repository navigation

connect creates new Props map object on every Action dispatch, which must be deeply compared #434

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

connect creates new Props map object on every Action dispatch, which must be deeply compared #434

Description

Activity

dave-doty commented on Dec 11, 2019

greglittlefield-wf commented on Dec 19, 2019

dave-doty commented on Dec 19, 2019

dave-doty commented on Dec 19, 2019

dave-doty commented on Dec 19, 2019

greglittlefield-wf commented on Dec 19, 2019

dave-doty commented on Dec 19, 2019

dave-doty commented on Dec 28, 2019

dave-doty commented on Dec 28, 2019

greglittlefield-wf commented on Feb 17, 2020

dave-doty commented on Feb 21, 2020

aaronlademann-wf commented on Jul 17, 2020

dave-doty commented on Jul 18, 2020

dave-doty commented on Jul 18, 2020

aaronlademann-wf commented on Jul 18, 2020

dave-doty commented on Jul 18, 2020

dave-doty commented on Jul 18, 2020

aaronlademann-wf commented on Jul 22, 2020

dave-doty commented on Jul 22, 2020

aaronlademann-wf commented on Jul 22, 2020

aaronlademann-wf commented on Jul 22, 2020

dave-doty commented on Jul 22, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions