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

Support Dart_TypedDataAcquireData of Unmodifiable TypedData classes #42785

Closed
zanderso opened this issue Jul 21, 2020 · 10 comments
Closed

Support Dart_TypedDataAcquireData of Unmodifiable TypedData classes #42785

zanderso opened this issue Jul 21, 2020 · 10 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. customer-flutter library-typed-data type-enhancement A request for a change that isn't a bug

Comments

@zanderso
Copy link
Member

It is useful for embedders to be able to expose read-only memory regions to Dart code as external type data. This will let Flutter avoid expensive copies on the Dart thread of large amounts of image data, for example see here:

https://github.com/flutter/engine/blob/18fee597ab6ec957e6311ba2bc74d4732ad55bd7/lib/ui/painting/image_encoding.cc#L48

for Image.toByteData:

https://github.com/flutter/engine/blob/18fee597ab6ec957e6311ba2bc74d4732ad55bd7/lib/ui/painting.dart#L1610

(The caller of that API could then do a copy to a writable buffer if they need one.)

The best match for this in the Dart SDK appears to be the unmodifiable typed data view classes like UnmodifiableByteDataView. Unfortunately, when these buffers come back into the embedder for various reasons, a call to Dart_TypedDataAcquireData() will fail by returning an error because the Unmodifiable typed data classes don't pass the tests here:

if (!IsExternalTypedDataClassId(class_id) &&
.

This issue is a feature request to integrate the Unmodifiable typed data classes more seamlessly with the VM so that they're a good match for the read-only buffers we can expose from Flutter as an optimization. If there's another approach that would solve the problem, then I'm interested in that, too.

/cc @a-siva @rmacnak-google @dnfield

@zanderso zanderso added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-typed-data customer-flutter type-enhancement A request for a change that isn't a bug labels Jul 21, 2020
@a-siva
Copy link
Contributor

a-siva commented Jul 21, 2020

Since the embedder is creating External TypedData objects it should have direct access to the memory region, why does it need to call Dart_TypedDataAcquireData to access underlying buffer.

@rmacnak-google
Copy link
Contributor

With the existing API, it is possible to extract the underlying buffer with Dart_GetField and pass that to Dart_TypedDataAcquireData. Though it does seem reasonable to expect Dart_TypedDataAcquireData to handle all the UnmodifiableX that are defined in dart:typed_data.

@zanderso
Copy link
Member Author

Since the embedder is creating External TypedData objects it should have direct access to the memory region, why does it need to call Dart_TypedDataAcquireData to access underlying buffer.

The embedder can generate an external typed data object, and return it to Dart code, then that object can come back into the embedder through a Dart language API. A slightly contrived example would be the return from Image.toByteData() being passed back into Image.memory().

With the existing API, it is possible to extract the underlying buffer with Dart_GetField()

Yeah, it looks like a vm:entry-point annotation would be needed. I would also worry about the implications of reaching into the core libraries and grabbing a private field.

@rmacnak-google
Copy link
Contributor

@mkustermann What do you think about giving the unmodifiable views fixed CIDs like the regular typed data views?

@mkustermann
Copy link
Member

@mkustermann What do you think about giving the unmodifiable views fixed CIDs like the regular typed data views?

Right now I can discourage the usage of the unmodifiable views for the following reason:

The canonical representation of bytes in Dart code is Uint8List. All our Dart code uses Uint8List to access bytes (e.g. protobuf encoder/decoder, http parser, ...). We want to make reading / writing of bytes as fast as possible by hoisting bounds checks out of loops and directly accessing the bytes (i.e. no method calls).

Optimized access: Our AOT compiler does perform those optimizations - accessing Uint8List is around two machine instructions - as long as there are no 3rd party implementations of the Uint8List interface (**). We can do that due to our sound type system. (The JIT has dynamic type feedback and doesn't need to rely on this.)

3rd party implementations: The UnmodifiableUint8ListView class is right now tree-shakable. If Dart code does not use it we get the benefits mentioned above. If Dart code uses it, these optimizations are disabled in AOT. Call sites become polymorphic, any code working with bytes becomes significantly slower.

Short term option: We can turn those unmodifiable view classes to be VM-supported and use our unified typed data layout. That would allow us to have fast read access. BUT: The class would no longer be tree shakable and the Uint8List.[]= write access would always be polymorphic and suffer quite bad performance. To alleviate this bad performance, we could change our AOT optimization to specialize the code conditionally, i.e. if (ClassID.get(obj) is unmodifiable-view) { Call []= method; } else { store bytes directly }. Though it would still be slower and bigger.

Longer term option: I have talked to members of the language/library team about this over the last few months and suggested we could solve it via: a) Introduce a new ReadOnlyUint8List interface that does not implement the writable Uint8List interface. b) add VM-supported read-only class (which flutter could e.g. use) c) disallow 3rd party implementations (similar to int) d) migrate existing code to only . This would allows fast read access, but also fast write access). Though it might require re-writing a lot of library code to use ReadOnlyUint8List instead of Uint8List.

=> If we do the optimizations mentioned in the short term option above and take the performance hit for write-access we could go ahead. @sstrickl or @askeksa-google could take a look at this.

(**) Our internal implementations are _Uint8List, _Uint8ArrayView, _ExternalUint8Array. They have a unified layout that allows accessing bytes directly.

@dnfield
Copy link
Contributor

dnfield commented Jul 22, 2020

We did just introduce an ImmutableBuffer class for dart:ui. Perhaps that would help here?

@zanderso
Copy link
Member Author

Thanks for looking into this @mkustermann!

@dnfield Currently, the dart:ui ImmutableBuffer is neither readable nor writable ;-). To make it readable, a reference to the buffer would be cached in a field, and the type for that field that seems like the best fit would be an Unmodifiable typed data view. Alternately, if Unmodifiable typed data views had the right properties, we could investigate using them instead of introducing a new class for a somewhat similar purpose.

@dnfield
Copy link
Contributor

dnfield commented Jul 22, 2020

It would probably help to understand the use cases we'd want to support with this.

If the goal is to avoid unnecessary copies, we'd want to know what we're eventually copying to and from - for example, if it's a file or a socket, users may not ever need to get access to the bytes directly in Dart, right?

If they need to do comparisons, we could probably have interfaces that boil down to doing a memcmp against a Uint8List after we acquire the typed data.

But if Dart could handle Unmodifiable more efficiently and avoid copies that would be a good thing, since you could then more easily compose Dart based APIs for this.

@jonahwilliams
Copy link
Contributor

Another use case is in flutter/engine#21153: we currently have to copy the memory mapped bytes of all assets (potentially quite large!) in order to pass them back through the engine. If we could directly convert them to read only buffers, we'd be able to avoid tons of work

@mkustermann
Copy link
Member

To speed up the discussion on this, I've written a proposal (see go/performant-typeddata-in-dart). If we follow this path we would ensure to have always fast access to bytes while safely allowing immutable bytes as well as sharing of such across isolates.

copybara-service bot pushed a commit that referenced this issue Aug 9, 2022
These types now work with Dart_TypedDataAcquireData.

The presence of these types no longer degrades the performance of typed data indexed loads.

The presence of these types degrades the performance of typed data indexed stores much less. The performance of indexed stores is somewhat regressed if these types were not used.

TEST=ci
Bug: #32028
Bug: #40924
Bug: #42785
Change-Id: I05ac5c9543f6f61ac37533b9efe511254778caed
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253700
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
copybara-service bot pushed a commit that referenced this issue Aug 10, 2022
This reverts commit d1112d3.

Reason for revert: b/242043014

Original change's description:
> [vm] Recognize unmodifiabled typed data views.
>
> These types now work with Dart_TypedDataAcquireData.
>
> The presence of these types no longer degrades the performance of typed data indexed loads.
>
> The presence of these types degrades the performance of typed data indexed stores much less. The performance of indexed stores is somewhat regressed if these types were not used.
>
> TEST=ci
> Bug: #32028
> Bug: #40924
> Bug: #42785
> Change-Id: I05ac5c9543f6f61ac37533b9efe511254778caed
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/253700
> Reviewed-by: Aske Simon Christensen <askesc@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>
> Commit-Queue: Ryan Macnak <rmacnak@google.com>

TBR=kustermann@google.com,rmacnak@google.com,askesc@google.com

TEST=ci
Change-Id: I32c1c460fc30c51bc0d42e7cfaafe72bf5630069
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #32028
Bug: #40924
Bug: #42785
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254560
Reviewed-by: Siva Annamalai <asiva@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
copybara-service bot pushed a commit that referenced this issue Aug 11, 2022
These types now work with Dart_TypedDataAcquireData.

The presence of these types no longer degrades the performance of typed data indexed loads.

The presence of these types degrades the performance of typed data indexed stores much less. The performance of indexed stores is somewhat regressed if these types were not used.

TEST=ci
Bug: #32028
Bug: #40924
Bug: #42785
Change-Id: Iffad865708501acf96db418985cd5a69bd9afa55
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/254501
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. customer-flutter library-typed-data type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants