-
Notifications
You must be signed in to change notification settings - Fork 6k
Channel buffers #12167
Channel buffers #12167
Changes from 7 commits
12c4f89
8a8343d
cff180e
31a845b
c28999d
4253c1c
e19f59c
63f7b8a
4e87cb0
24607ab
c4bec51
ea3823f
afc09b6
0a3c609
9df8b23
f32af48
97be8ca
d041d62
1533428
42c742d
652a822
4d84d1b
33c21e9
afa7bb0
6b6dbb9
bcc5ea7
3ff518c
8d4a1e4
8e6ee3b
3e3c38d
a4204c9
1527c3b
837ed7e
23a9ce9
007edb1
741981b
571594e
478520f
8569f21
96761f8
adcd641
24cf927
67aecdb
b13a3dc
282bff0
a0a4c0e
26d9deb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| part of dart.ui; | ||
|
|
||
| /// A saved platform message for a channel with its callback. | ||
| class StoredMessage { | ||
| final ByteData _data; | ||
| final PlatformMessageResponseCallback _callback; | ||
|
|
||
| StoredMessage(this._data, this._callback); | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| ByteData get data => _data; | ||
|
gaaclarke marked this conversation as resolved.
|
||
| PlatformMessageResponseCallback get callback => _callback; | ||
|
gaaclarke marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /// A fixed-size circular queue. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A per channel, fixed-size circular queue.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| class _RingBuffer<T> { | ||
| final collection.ListQueue<T> _queue; | ||
| int _capacity; | ||
|
|
||
| _RingBuffer(this._capacity) | ||
| : _queue = collection.ListQueue<T>(_capacity); | ||
|
|
||
| int get length => _queue.length; | ||
|
|
||
| int get capacity => _capacity; | ||
|
|
||
| bool get isEmpty => _queue.isEmpty; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doc
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't say anything :) https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-useless-documentation Something like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, this works. |
||
| /// Returns true on overflow. | ||
| bool push(T val) { | ||
| bool overflow = false; | ||
| while (_queue.length >= _capacity) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't you want to reuse the resize logic here or extract a part of it? There's some repetition here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| _queue.removeFirst(); | ||
| overflow = true; | ||
| } | ||
| _queue.addLast(val); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work if the capacity is zero? Add test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| return overflow; | ||
| } | ||
|
|
||
| /// Returns null when empty. | ||
| T pop() { | ||
| return _queue.isEmpty ? null : _queue.removeFirst(); | ||
| } | ||
|
|
||
| /// Returns the number of discarded items resulting from resize. | ||
| int resize(int newSize) { | ||
| int result = 0; | ||
|
|
||
| while (length > newSize) { | ||
| result += 1; | ||
| _queue.removeFirst(); | ||
| } | ||
|
|
||
| _capacity = newSize; | ||
|
|
||
| return result; | ||
| } | ||
| } | ||
|
|
||
| /// Storage of channel messages until the channels are completely routed. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. completely routed upon attaching a channel message handler on the framework side.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| class ChannelBuffers { | ||
|
gaaclarke marked this conversation as resolved.
|
||
| /// A somewhat arbitrary size that tries to balance handling typical | ||
| /// cases and not wasting memory. | ||
| static const int kDefaultBufferSize = 100; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems arbitrary. i would expect this to be 0, 1, or infinity.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're not going to be able to have a 1 size fits all solution. Users instantiating their own channels should be able to specify the caching behavior per channel.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would a user control this though? A message could be multiple megabytes of data (e.g. what I did in the scenario app to get the timeline data from dart back to Java). Or it could be a single byte. If we default this to 1 for now, it would at least solve the "variables" use case - but it would be nice if we can come up with some way for users to say how many messages their channel shoul dbe allowed to buffer.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dnfield I plan resizing for a future PR. I've since moved the default size to 1. |
||
|
|
||
| final Map<String, _RingBuffer<StoredMessage>> _messages = {}; | ||
|
|
||
| /// Returns true on overflow. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each field of a dart class has its own dartdoc page. For example https://api.flutter.dev/flutter/dart-ui/Offset/infinite-constant.html It needs to be sufficiently complete to stand on its own.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't see what you are referring to in this comment.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant each field's doc needs to fully describe what they do, when they're used etc without the context of any surrounding fields' docs or the class doc since they appear independently when we generate dartdoc pages
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I added documentation even on private fields.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't mean adding documentations on private fields. I just meant each public field of public classes (appearing in their own URL with their own page in the API doc website) should fully describe themselves. Consider, as example, seeing https://screenshot.googleplex.com/K4iw8SwzUuE vs https://screenshot.googleplex.com/QxCLRcoZFqY |
||
| bool push(String channel, ByteData data, PlatformMessageResponseCallback callback) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this API futureproof enough? When each channel can specify its own cache size, how would it express it? I assume via the channel constructor. When it does so, how does it pass that size int into this signature? Would we need to leak the implementation detail of this class's implementation into the channel class implementation (by forcing it to come call ChannelBuffers.resize)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe so. For basic channels the constructor will call This comment is on the "push" method, I'm guessing it was suppose to be somewhere else.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, SG |
||
| _RingBuffer<StoredMessage> queue = _messages[channel]; | ||
| if (queue == null) { | ||
| queue = _RingBuffer<StoredMessage>(kDefaultBufferSize); | ||
| _messages[channel] = queue; | ||
| } | ||
| final bool result = queue.push(StoredMessage(data, callback)); | ||
| if (result) { | ||
| _Logger._printString('Overflow on channel:' + channel); | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| } | ||
| return result; | ||
| } | ||
|
|
||
| /// Returns null on underflow. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would this happen? Wouldn't consumers check isEmpty first?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should be calling isEmpty. You know how it is, troublemakers don't follow the protocol.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dem delinquent kids these days |
||
| StoredMessage pop(String channel) { | ||
| final _RingBuffer<StoredMessage> queue = _messages[channel]; | ||
| final StoredMessage result = queue?.pop(); | ||
| if (result == null) { | ||
| _Logger._printString('Underflow on channel:' + channel); | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| } | ||
| return result; | ||
| } | ||
|
|
||
| bool isEmpty(String channel) { | ||
| final _RingBuffer<StoredMessage> queue = _messages[channel]; | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| return queue?.isEmpty ?? true; | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| void resize(String channel, int newSize) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All public Dart classes and fields need dartdocs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
| _RingBuffer<StoredMessage> queue = _messages[channel]; | ||
| if (queue == null) { | ||
| queue = _RingBuffer<StoredMessage>(newSize); | ||
| _messages[channel] = queue; | ||
| } else { | ||
| queue.resize(newSize); | ||
| } | ||
| } | ||
|
gaaclarke marked this conversation as resolved.
|
||
| } | ||
|
|
||
| final ChannelBuffers channelBuffers = ChannelBuffers(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably not be a static but owned by someone. When there are multiple engines, each should (via some chain of ownership) end up with a different instance of buffers.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't each engine have its own isolate and therefore its own channelBuffers? If this is true this will also be a problem for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup you're right, that was a nonsensical comment :D
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still needs a doc (for public fields)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,7 +161,9 @@ void _dispatchPlatformMessage(String name, ByteData data, int responseId) { | |
| }, | ||
| ); | ||
| } else { | ||
| window._respondToPlatformMessage(responseId, null); | ||
| channelBuffers.push(name, data, (ByteData responseData) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to the comment on the other PR. I think there are 2 lifecycles and we can't really conflate them. There's setting the onPlatformMessage which happens once per binding, and there's adding per channel handlers which happens at arbitrary time. Once the onPlatformMessage is attached, all buffers will stop queueing. But there are no guarantee that the following sequence of actions won't happen. 1- run entrypoint, entrypoint runApps, which attaches window.onPlatformMessage. It probably works now since the synchronous execution of runApp+binding constructor+our system channels setting handler all happens at the same time. But we can't guarantee that for users' own channels.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've addressed this in the other PR. Both cases get queued up now. |
||
| window._respondToPlatformMessage(responseId, responseData); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| import 'dart:ui' as ui; | ||
| import 'dart:typed_data'; | ||
| import 'dart:convert'; | ||
|
|
||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
|
gaaclarke marked this conversation as resolved.
|
||
|
|
||
| ByteData _makeByteData(String str) { | ||
| var list = utf8.encode(str); | ||
| var buffer = list is Uint8List ? list.buffer : new Uint8List.fromList(list).buffer; | ||
| return ByteData.view(buffer); | ||
| } | ||
|
|
||
| String _getString(ByteData data) { | ||
| final buffer = data.buffer; | ||
| var list = buffer.asUint8List(data.offsetInBytes, data.lengthInBytes); | ||
| return utf8.decode(list); | ||
| } | ||
|
|
||
| test('push pop', () async { | ||
| String channel = "foo"; | ||
| ByteData data = _makeByteData('bar'); | ||
| ui.ChannelBuffers buffers = ui.ChannelBuffers(); | ||
| ui.PlatformMessageResponseCallback callback = (ByteData responseData) {}; | ||
| buffers.push(channel, data, callback); | ||
| ui.StoredMessage storedMessage = buffers.pop(channel); | ||
| expect(storedMessage.data, equals(data)); | ||
| expect(storedMessage.callback, equals(callback)); | ||
| }); | ||
|
|
||
| test('empty', () async { | ||
| String channel = "foo"; | ||
| ByteData data = _makeByteData('bar'); | ||
| ui.ChannelBuffers buffers = ui.ChannelBuffers(); | ||
| ui.PlatformMessageResponseCallback callback = (ByteData responseData) {}; | ||
| expect(buffers.isEmpty(channel), equals(true)); | ||
| buffers.push(channel, data, callback); | ||
| expect(buffers.isEmpty(channel), equals(false)); | ||
| }); | ||
|
|
||
| test('pop', () async { | ||
| ui.ChannelBuffers buffers = ui.ChannelBuffers(); | ||
| expect(buffers.pop("channel"), equals(null)); | ||
| }); | ||
|
|
||
| test('overflow', () async { | ||
| String channel = "foo"; | ||
| ByteData one = _makeByteData('one'); | ||
| ByteData two = _makeByteData('two'); | ||
| ByteData three = _makeByteData('three'); | ||
| ByteData four = _makeByteData('four'); | ||
| ui.ChannelBuffers buffers = ui.ChannelBuffers(); | ||
| ui.PlatformMessageResponseCallback callback = (ByteData responseData) {}; | ||
| buffers.resize(channel, 3); | ||
| expect(buffers.push(channel, one, callback), equals(false)); | ||
| expect(buffers.push(channel, two, callback), equals(false)); | ||
| expect(buffers.push(channel, three, callback), equals(false)); | ||
| expect(buffers.push(channel, four, callback), equals(true)); | ||
| expect(_getString(buffers.pop(channel).data), equals('two')); | ||
| }); | ||
| } | ||
|
gaaclarke marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.