Channel buffers #12167

gaaclarke · 2019-09-10T16:32:33Z

Started storing messages for channels sent before the engine is properly setup for handling messages. A second part of this work will be draining the channel buffers inside the framework.

Related issue: flutter/flutter#38914
Related Design Doc (internal only, sorry): https://docs.google.com/document/d/10PEzCpY12P0se7cApopg_RCDJXeCqsr9AicNzsxU3Ds/edit#

go until message handling is setup.

dnfield

Let's try to limit the amount of public API we add for this as much as possible. We should also look at the docs that we have that are user facing, as well as any warnings/errors we print.

/cc @Hixie since this is adding public surface API to dart:ui

lib/ui/channel_buffers.dart

testing/dart/channel_buffers_test.dart

yjbanov

The web parts LGTM

lib/ui/channel_buffers.dart

xster

Sorry, there's a bunch of comments, but mostly around the rigor needed for public Dart APIs

xster · 2019-09-16T22:49:14Z

lib/ui/channel_buffers.dart

+  PlatformMessageResponseCallback get callback => _callback;
+}
+
+/// A fixed-size circular queue.


A per channel, fixed-size circular queue.

There isn't anything about the implementation that requires this to be used for channels. It's a generic RingBuffer that could be used anywhere.

xster · 2019-09-16T22:49:29Z

lib/ui/channel_buffers.dart

+class _RingBuffer<T> {
+  final collection.ListQueue<T> _queue;
+  int _capacity;
+  Function(T) _dropItemCallback;


Not clear what this should be. Add doc.

Done, missed responding to this last night.

xster · 2019-09-16T22:49:43Z

lib/ui/channel_buffers.dart

+  int get capacity => _capacity;
+
+  bool get isEmpty => _queue.isEmpty;
+


This doesn't say anything :) https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-useless-documentation

Something like Add a callback when previously queued messages are being dropped due to overflow. The message instance is returned in the callback..

That seemed redundant since the docstring for _dropItemCallback says all of that. The intent was to point them to that documentation and not repeat it.

I found examples elsewhere and they put the ivar next to the setter and getter and it gets collapsed in the generated documentation so I don't have to repeat it, so done

Yup, this works.

xster · 2019-09-16T22:53:03Z

lib/ui/channel_buffers.dart

+  /// Returns true on overflow.
+  bool push(T val) {
+    bool overflow = false;
+    while (_queue.length >= _capacity) {


wouldn't you want to reuse the resize logic here or extract a part of it? There's some repetition here.

Done, it wasn't trivial since the logic was a bit different and didn't buy many lines, but since you asked there you go.

xster · 2019-09-16T22:53:46Z

lib/ui/channel_buffers.dart

+  }
+}
+
+typedef DrainChannelCallback = Future<void> Function(ByteData, PlatformMessageResponseCallback);


Doc. Who is supposed to use it. Use See also: for references.

xster · 2019-09-16T23:07:31Z

lib/ui/channel_buffers.dart

+                           'Messages on this channel are being sent faster '
+                           'than they are being processed which is resulting '
+                           'in the dropping of messages.  The engine may not be '
+                           'running or you need to adjust the buffer size.');


I would gate adjusting the buffer size with a lot of care. It's rarely what the user really wants to do. Make sure we thoroughly explain the cause before recommending this.

xster · 2019-09-16T23:08:00Z

lib/ui/channel_buffers.dart

+    return result;
+  }
+
+  /// Returns null on underflow.


When would this happen? Wouldn't consumers check isEmpty first?

They should be calling isEmpty. You know how it is, troublemakers don't follow the protocol.

dem delinquent kids these days

xster · 2019-09-16T23:09:13Z

lib/ui/channel_buffers.dart

+    return (queue == null) ? true : queue.isEmpty;
+  }
+
+  void resize(String channel, int newSize) {


All public Dart classes and fields need dartdocs.

xster · 2019-09-16T23:28:40Z

lib/web_ui/lib/src/ui/channel_buffers.dart

+
+part of ui;
+
+typedef DrainChannelCallback = Future<void> Function(ByteData, PlatformMessageResponseCallback);


I'm not sure what you are referring to. I updated the docstring to match the ones found in lib/ui/window.dart

xster · 2019-09-16T23:29:34Z

lib/web_ui/lib/src/ui/channel_buffers.dart

+/// Storage of channel messages until the channels are completely routed,
+/// i.e. when a message handler is attached to the channel on the framework side.
+///
+/// Each channel has a finite buffer capacity and in a FIFO manner messages will


Make this doc correct (i.e. this class doesn't do anything on web)

xster · 2019-09-17T11:40:34Z

lib/ui/channel_buffers.dart

+      queue = _makeRingBuffer(kDefaultBufferSize);
+      _messages[channel] = queue;
+    }
+    final bool result = queue.push(_StoredMessage(data, callback));


just call this overflowed locally if that's what it's supposed to mean

xster · 2019-09-17T11:42:00Z

lib/ui/channel_buffers.dart

+      // TODO(aaclarke): Update this message to include instructions on how to resize
+      // the buffer once that is available to users.
+      _Logger._printString('Overflow on channel: $channel.  '
+                           'Messages on this channel are being discarded.  '


The temporal reference isn't clear. Make it clear that it's previously cached messages that are being discarded.

Added FIFO notice. I don't want to say previous cached methods because if the size is zero, that is misleading.

xster · 2019-09-17T11:42:26Z

lib/ui/channel_buffers.dart

+
+  /// Changes the capacity of the queue associated with the given channel.
+  ///
+  /// This could result in the dropping of messages if the newSize is less


nit newSize

No article? Done.

ha, I'm sure there was a goomics episode about this but I can't find it anymore.

I actually meant use back-ticks ` around method argument references in docs.

xster · 2019-09-17T11:42:51Z

lib/ui/channel_buffers.dart

+
+  /// Changes the capacity of the queue associated with the given channel.
+  ///
+  /// This could result in the dropping of messages if the newSize is less


dropping of messages from the front of the queue

xster · 2019-09-17T11:43:23Z

lib/ui/channel_buffers.dart

+  /// Remove and process all stored messages for a given channel.
+  ///
+  /// This should be called once a channel is prepared to handle messages
+  /// (ie when a message handler is setup in the framework).


xster · 2019-09-17T11:43:54Z

lib/ui/channel_buffers.dart

+  }
+}
+
+final ChannelBuffers channelBuffers = ChannelBuffers();


Still needs a doc (for public fields)

xster

LGTM

printed in debug builds.

[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4 git log 63873d9..d1692d4 --no-merges --oneline 2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318) 2019-09-17 [email protected] README for the felt tool (flutter/engine#12323) 2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320) 2019-09-17 [email protected] Tests for #11283 (flutter/engine#12322) 2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229) 2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits) 2019-09-17 [email protected] Channel buffers (flutter/engine#12167) 2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128) 2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287) 2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319) 2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303) 2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316) 2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315) 2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313) 2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312) 2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306) 2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277) 2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307) 2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182) 2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304) 2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288) 2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

[email protected]:flutter/engine.git/compare/63873d9f421f...d1692d4 git log 63873d9..d1692d4 --no-merges --oneline 2019-09-17 [email protected] Update canvaskit backend (flutter/engine#12318) 2019-09-17 [email protected] README for the felt tool (flutter/engine#12323) 2019-09-17 [email protected] Fix continuous event polling in the GLFW event loop (flutter/engine#12320) 2019-09-17 [email protected] Tests for flutter#11283 (flutter/engine#12322) 2019-09-17 [email protected] Improve check to render (or not) a DRRect when inner falls outside of outer on RecordingCanvas (flutter/engine#12229) 2019-09-17 [email protected] Roll src/third_party/dart dd1969a43a..7505b3a5f0 (39 commits) 2019-09-17 [email protected] Channel buffers (flutter/engine#12167) 2019-09-17 [email protected] Make iOS FlutterViewController stop sending inactive/pause on app lifecycle events when not visible (flutter/engine#12128) 2019-09-17 [email protected] Adds PluginRegistry to the C++ client wrapper API (flutter/engine#12287) 2019-09-17 [email protected] Add "type" to getDisplayRefreshRate protocol (flutter/engine#12319) 2019-09-17 [email protected] Add a build command to felt (flutter/engine#12303) 2019-09-17 [email protected] Roll src/third_party/skia df432d5efb70..d545bfbb94ca (1 commits) (flutter/engine#12316) 2019-09-17 [email protected] Roll fuchsia/sdk/core/linux-amd64 from rwf0-... to RRgw-... (flutter/engine#12315) 2019-09-17 [email protected] Roll src/third_party/skia f8486f2c5fb6..df432d5efb70 (1 commits) (flutter/engine#12313) 2019-09-17 [email protected] Roll src/third_party/skia b47704b0bd34..f8486f2c5fb6 (2 commits) (flutter/engine#12312) 2019-09-16 [email protected] Fix the declaration of setSystemGestureExclusionRects to match the PlatformMessageHandler interface (flutter/engine#12306) 2019-09-16 [email protected] Manage resource and onscreen contexts using separate IOSGLContext objects (flutter/engine#12277) 2019-09-16 [email protected] Cleanup in web_ui (flutter/engine#12307) 2019-09-16 [email protected] Made flutter startup faster by allowing initialization to be parallelized (flutter/engine#10182) 2019-09-16 [email protected] Roll src/third_party/skia c22498502cda..b47704b0bd34 (16 commits) (flutter/engine#12304) 2019-09-16 [email protected] Include firefox in check to quote font families (flutter/engine#12288) 2019-09-16 [email protected] Roll src/third_party/dart 7799f424f4..dd1969a43a (2 commits) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

gaaclarke added 4 commits September 9, 2019 14:54

Added channel buffers to ui so that messages have a place to

12c4f89

go until message handling is setup.

Added the RingBuffer class.

8a8343d

Moved over to using the ring buffer.

cff180e

Did a bit of cleanup.

31a845b

googlebot added the cla: yes label Sep 10, 2019

gaaclarke requested review from xster and dnfield September 10, 2019 16:32

Removed trailing space.

c28999d

dnfield requested a review from Hixie September 10, 2019 17:36

dnfield suggested changes Sep 10, 2019

View reviewed changes

gaaclarke mentioned this pull request Sep 10, 2019

Channel buffers flutter/flutter#40165

Merged

5 tasks

gaaclarke added 4 commits September 10, 2019 12:24

Made RingBuffer private.

4253c1c

Fixed formatting and documenation on default ringbuffer size.

e19f59c

Updated the warning messages.

63f7b8a

Expanded on docstring.

4e87cb0

dnfield reviewed Sep 10, 2019

View reviewed changes

testing/dart/channel_buffers_test.dart Show resolved Hide resolved

gaaclarke added 9 commits September 10, 2019 13:09

Updated constructor location and docstring.

24607ab

Added extra test for resizing.

c4bec51

Switched to ternary operator since it's more clear.

ea3823f

added type annotation to literal

afc09b6

added new file to licenses_flutter

0a3c609

added channel buffers to the web ui.dart

9df8b23

added channel_buffers to web_ui

f32af48

For web_ui, replaced ListQueue with Queue and removed log statements.

97be8ca

fixed collections reference

d041d62

gaaclarke requested a review from yjbanov September 10, 2019 21:34

gaaclarke added 2 commits September 10, 2019 14:41

Added web_ui file to licenses_flutter

1533428

fixed web_ui "part of" declaration

42c742d

yjbanov approved these changes Sep 10, 2019

View reviewed changes

Hixie reviewed Sep 11, 2019

View reviewed changes

lib/ui/channel_buffers.dart Show resolved Hide resolved

xster reviewed Sep 16, 2019

View reviewed changes

gaaclarke added 7 commits September 16, 2019 17:21

Merge branch 'master' into channel-buffers

007edb1

added some docstrings, removed duplication

741981b

added test for zero size

571594e

added docstring

478520f

update docstring and error message

8569f21

added docstringg

96761f8

Updated web_ui documentation

adcd641

xster reviewed Sep 17, 2019

View reviewed changes

gaaclarke added 6 commits September 17, 2019 10:20

shuffled around docstrings to collapse the setter and the ivar.

24cf927

added more documentation

67aecdb

updated variable name, tweaked log message

b13a3dc

updated docstrings

282bff0

Made it so that channel buffer's error messages are only

a0a4c0e

printed in debug builds.

fixed formatting of c++ code

26d9deb

dnfield approved these changes Sep 17, 2019

View reviewed changes

gaaclarke merged commit 0a455a8 into flutter:master Sep 17, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 17, 2019

0a455a8 Channel buffers (flutter/engine#12167)

2ce3baa

engine-flutter-autoroll mentioned this pull request Sep 17, 2019

Roll engine 63873d9f421f..d1692d4cc703 (22 commits) flutter/flutter#40717

Merged

gaaclarke mentioned this pull request Sep 23, 2019

add2app model swift Platform Channel not working flutter/flutter#39967

Closed

xster mentioned this pull request Mar 2, 2020

Easy to use the wrong binaryMessenger handler set pattern on iOS flutter/flutter#39870

Closed

xster mentioned this pull request Jul 11, 2020

setInitialRoute is broken for iOS add-to-app flutter/flutter#59895

Closed

		int get capacity => _capacity;

		bool get isEmpty => _queue.isEmpty;


		part of ui;

		typedef DrainChannelCallback = Future<void> Function(ByteData, PlatformMessageResponseCallback);

Channel buffers #12167

Channel buffers #12167

Conversation

gaaclarke commented Sep 10, 2019

dnfield left a comment

Choose a reason for hiding this comment

yjbanov left a comment

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster left a comment

Choose a reason for hiding this comment