Performance problem: ViewModel is unnecessarily calculated
marcglasberg opened this issue · 2 comments
The _StoreStreamListener listens to a stream of dispatched states:
void _createStream() {
_stream = widget.store.onChange
.where(_ignoreChange)
.map(_mapConverter)
.where(_whereDistinct)
.transform(
StreamTransformer.fromHandlers(handleData: _handleChange, handleError: _handleError));
}However, it doesn't actually uses these states to calculate the vm in _mapConverter. Granted, the _ignoreChange method uses the stream states, but it should not: #196
In other words, the states in the stream are being ignored by _mapConverter which instead uses the current store state:
ViewModel _mapConverter(S state) {
return widget.converter(widget.store);
}If find this very weird, but I can't think of a problem (once _ignoreChange is fixed, that is). The store state is guaranteed to be the same or more recent than the stream state. So it seems to me that by using the store state instead of the stream state we're managing to update the vm a few milliseconds early, with no other bad effects.
The stream in fact is only being used to signal to the widget that state changes have occurred, recently. This gives the widget a chance to recalculate its vm from the current store state, and rebuild if necessary. However, the stream changes are not in sync with the store state. This means the stream may be changing while the store state is not, and all those vm calculations are just a waste.
The following code demonstrates the problem:
import 'package:flutter/material.dart';
import 'package:flutter_redux/flutter_redux.dart';
import 'package:redux/redux.dart';
enum Actions { Increment }
int counterReducer(int state, dynamic action) {
if (action == Actions.Increment) return state + 1;
return state;
}
void main() {
final store = Store<int>(counterReducer, initialState: 0);
runApp(FlutterReduxApp(store: store));
}
class FlutterReduxApp extends StatelessWidget {
final Store<int> store;
final String title;
FlutterReduxApp({Key key, this.store, this.title}) : super(key: key);
@override
Widget build(BuildContext context) {
return StoreProvider<int>(
store: store,
child: MaterialApp(
title: 'Example',
home: Scaffold(
appBar: AppBar(title: const Text('Example')),
body: Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
Text(
'Each time you click it increments 5 times. '
'It should ignore odd results, but it does not:',
),
StoreConnector<int, String>(
converter: (store) {
print('Called the converter = ${store.state}');
return store.state.toString();
},
ignoreChange: (count) {
var isOdd = (count % 2 == 1);
print('ignoreChange? $isOdd (count = ${count})');
return isOdd;
},
builder: (context, count) {
return Text(count, style: Theme.of(context).textTheme.display1);
},
)
],
),
),
floatingActionButton: StoreConnector<int, VoidCallback>(converter: (store) {
// Increments 5 times.
return () {
store.dispatch(Actions.Increment);
store.dispatch(Actions.Increment);
store.dispatch(Actions.Increment);
store.dispatch(Actions.Increment);
store.dispatch(Actions.Increment);
};
}, builder: (context, callback) {
return FloatingActionButton(
onPressed: callback,
child: Icon(Icons.add),
);
}))));
}
}
If you run this, you will see this in the console:
I/flutter ( 8595): Called the converter = 10
I/flutter ( 8595): Called the converter = 10
I/flutter ( 8595): Called the converter = 10
I/flutter ( 8595): Called the converter = 10
I/flutter ( 8595): Called the converter = 10
The converter is being called 5 times by the stream, once for each store.dispatch with different stream states. However, the store state is the same, and is this state the one used by the converter.
I propose the _createStream method should be changed to:
void _createStream() {
_stream = widget.store.onChange
.where(_stateChanged)
.where(_ignoreChange)
.map(_mapConverter)
.where(_whereDistinct)
.transform(
StreamTransformer.fromHandlers(handleData: _handleChange, handleError: _handleError));
}Where the _stateChanged method is given by:
S _lastConvertedState;
...
// No need to recalculate the vm if the widget.store.state is identical.
bool _stateChanged(S state) {
var ifStateChanged = !identical(_lastConvertedState, widget.store.state);
_lastConvertedState = widget.store.state;
return ifStateChanged;
}Continuing: The above fix is a low hanging fruit, and has the potential to greatly reduce the number of unnecessary view-model creations. I have already applied it to async_redux, and I believe it should be applied to flutter_redux too. See: PR #198
However, this is still not perfect. In the build method we find this:
@override
Widget build(BuildContext context) {
return widget.rebuildOnChange
? StreamBuilder<ViewModel>(
stream: _stream,
builder: (context, snapshot) {
if (_latestError != null) throw _latestError;
return widget.builder(
context,
_latestValue,
);
},
)
: _latestError != null
? throw _latestError
: widget.builder(context, _latestValue);
}
The StreamBuilder may be called 60fps, and it will use the most recent _latestValue (it would be better to call this _latestVm). It will NOT be called every time the model changes. In other words, it may be called at most 60 times per second (each 16 milliseconds aprox.), and if the stream has a new value it will use the most recent _latestValue.
If the _latestValue has been calculated 100 times in the last 16 ms, this means 99 useless vm calculations.
It would be much better if we could check if a new value is available, and then, only if it is, we calculate the _latestValue, compare it with the previous one (if the distinct parameter is true).
This change would guarantee that we only calculate the vm if there is at least a chance we'll use it.
Thanks again, @marcglasberg. I'll close this one with the fix merged!