brianegan / dart_redux_epics

Redux.dart middleware for handling actions using Dart Streams
MIT License
141 stars 22 forks source link

Performance issue of Future.delayed hack #14

Closed ranquild closed 6 years ago

ranquild commented 6 years ago

This line: https://github.com/brianegan/dart_redux_epics/blob/master/lib/src/epic_middleware.dart#L50

runs timer on every action dispatched within store. In our app it adds visible performance bottleneck. Can you please add option to dispatch actions immediately (can be done backward-compatible)? We could use this option and do not use async* actions instead.

We are still using 0.7.x version and we will use it for few months from now (dart 2.0 migration). Can you also please publish this option for latest 0.7.x package as well?

brianegan commented 6 years ago

Sure, now that Dart2 is supporting this use-case a bit better, I'll see if I can actually just remove it.

NevRA commented 6 years ago

@brianegan any updates? Same performance issue with 0.7.x

brianegan commented 6 years ago

Hey all -- I tried simply removing this and it didn't quite work out :/ Therefore, I'd be happy to put an option in there to skip this step if you're not using async* functions at all, and see if I can find a better fix for this in the long-run.

@NevRA or @ranquild Do you happen to have any performance profiles you could share? It's really hard to debug performance issues without a concrete example that I can run with the observatory enabled for digging into the issue and seeing where it's slowing down, and the examples I've got on my side don't seem to have major perf issues.

I'm wondering if it's actually this hack that's slowing something down, or something different based on the fact that 0.7.x didn't fix the issue for @NevRA.

ranquild commented 6 years ago

@brianegan It seems that I've found easy solution to this issue without deleting hack completly:

if (_flag) {
  _actions.add(action);
} else {
  new Future.delayed(Duration.ZERO, () {
    _actions.add(action);
    _flag = true;
  });
}

Tested it in my view and it seems to work, however there are failing tests in this library because action can be dispatched sooner than expected.

Can you please look at this?

About perf tests, I will try extract code to simulate problem. Problem is that someone is dispatching actions to store on mouse move or similar.

brianegan commented 6 years ago

Thanks @ranquild -- hrm, this is an interesting problem overall. When I removed the Future.delayed hack, I notice other things break as well, such as the TypedEpic helpers, so I'm actually not sure a simple _flag solution will quite work.

I'm trying to see if I can rewrite the middleware a bit to support all of these use-cases without any hacks that are causing the performance issues.

Update -- wait, I'm an idiot -- forgot I was using async* functions for the typed epics... I refactored that out and can probably just use a flag like allowAsyncFunctions.

Wish I could still find an appropriate solution that supports everything -- it's truly odd

brianegan commented 6 years ago

Ok, I've published 0.10.2 & 0.7.1, which includes an option to disable support for async* functions until Dart 2 properly supports this use case via https://github.com/dart-lang/sdk/issues/33818

Unfortunately, the Dart team only implemented the needed functionality for async functions, rather than async* functions as well.

To disable support for async* functions, you can create your EpicMiddleware like this:

new EpicMiddleware<String>(
  myEpic,
  supportAsyncGenerators: false,
)