Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use streams because this is literally just a stream recreation #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

miyoyo
Copy link

@miyoyo miyoyo commented Jul 6, 2019

Since streams are backed by the VM, they're even faster.

@xenoken
Copy link
Owner

xenoken commented Jul 9, 2019

Hey @miyoyo! Thank you very much for your interest and for your pull request. I was already working on a new stream-based implementation, and now it is available in the master branch.

Vastly faster than wrapping the callback, and, since we create the streamcontroller right above, the `as` is safe.
@miyoyo
Copy link
Author

miyoyo commented Jul 9, 2019

Thanks, I'll still put my approach forward for a few reasons:

  1. Your current implementation is broken, creating a broadcast stream listens to the first stream. Fetching anything a second time breaks the code.

  2. The send API was broken without bumping a major revision, this is terrible form on pub.
    Even if you don't accept my pull request, I STRONGLY encourage you to repost the original as 1.0.4, then repost the current version as 2.0.0, and annotate the differences explicitly

  static void send<T>(T message) {
    _mainstream.add(message);
  }

vs

  static bool send<T>(T message) {
    final target = _instances[T];
    target?.add(message);
    return target != null;
  }
  1. Superfluous streams are created multiple times
  static fetch<T>(Function(T arg) func) {
    if (!_streams.containsKey(T)) {
      _streams[T] = _mainstream.stream
          .asBroadcastStream() // This creates a new stream controller
          .where((event) => event is T) // This too
          .cast<T>(); // So does this
    }

    _streams[T].listen((event) {
      func(event);
    });
  }
  1. Code that does more, is slower. With the latest patch I submitted to this pull request, even with send returning a bool, it is significantly faster

This is the test rig used for both versions

  final s = Stopwatch();
  final loops = 1000000;
  var target = 0;
  final complete = Completer<void>();
  s.start();
  A.fetch((int i) {
    target++;
    if (target >= loops) {
      complete.complete();
    }
  });
  for (var i = 0; i < loops; i++) {
    A.send(0);
  }
  await complete.future;
  s.stop();
  print("A done: ${s.elapsedMicroseconds}, at ${s.elapsedMicroseconds / loops} per loop on average");
I/flutter ( 6897): A done: 2636963, at 2.636963 per loop on average
I/flutter ( 6897): B done: 3089097, at 3.089097 per loop on average

Where A is this pull request, and B is the current master.
Keep in mind this is only with a single listener, which, while nearly insignificant for this pull request (Map doesn't slow down a lot as it gets bigger), Your approach requires that at least two streams per listener be hit for every single message created (First is the broadcast stream, second is the casting stream) making the performance significantly worse as more listeners come.

@xenoken
Copy link
Owner

xenoken commented Jul 10, 2019

Thanks for your insights. Based on your feedback, I made some edits to the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants