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

Check if TypedDataSpecializer misses any important cases on internal apps due to UnmodifiableUint8ListView instantiation #40924

Closed
mraleph opened this issue Mar 9, 2020 · 7 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-performance Issue relates to performance or code size

Comments

@mraleph
Copy link
Member

mraleph commented Mar 9, 2020

Turns out UnmodifiableUint8ListView is used internally as an immutable wrapper around embedded data.

This means TypedDataSpecializer would not inline any virtual accesses through Uint8List interface - because UnmodifiableUint8ListView introduces a third-party implementor which does not follow uniform _TypedData layout.

We should check if we leaving calls on any hot code paths due to this - and look into potentials ways to mitigate it.

/cc @mkustermann

@mraleph mraleph added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-performance Issue relates to performance or code size labels Mar 9, 2020
@mkustermann
Copy link
Member

This is a known issue, because that class is considered a 3rd party implementation of Uint8List, thereby disabling our AOT optimizations.

We could make it a 1st party implementation of Uint8List and make it use the unified layout. Though that would only make the reading interface fast. We would still need to disable the AOT optimizations for the writing part, because []= is implemented differently (it throws).

So rather than doing that, I would suggest we introduce a ReadOnlyUint8List interface, which Uint8List implements. That would give users an actual Dart type to use in code where bytes shouldn't be modified.

The UnmodifiableUint8ListView can then implement ReadOnlyUint8List. Downcasts to Uint8List would fail and we could therefore continue to optimize write-interface operations on Uint8List.

/cc @lrhn Can we do this?

@mkustermann
Copy link
Member

In addition to this we would like to make the typed data classes be non-implementable by 3rd party users.

@mkustermann
Copy link
Member

This would also be a stepping stone for us to eventually add support for having compile-time constant byte arrays (e.g. const ReadOnlyUint8List(const <int>[1, ...]))

@lrhn
Copy link
Member

lrhn commented Mar 9, 2020

We generally do not have any "ReadOnly" collection interfaces, and I don't think we'll start now.

If we did that, we should probably have the same for List, Set and Map as well, and potentially a number of other lookup-allowing collections (other than Iterable).

If we did introduce a smaller read-only super-interface for Uint8List (implementing the smaller super-interface of List), then code would have to start using it. Changing any existing API is a breaking change (if it's used as an interface anywhere, someone might implement that interface and require a Uint8List, or if it's used as a return type, someone might be modifying the currently returned value). That makes it basically impossible to use the new type in the existing APIs.
If we don't add the ReadOnlyList superinterface of List then a ReadOnlyUint8List won't be a List any more. That'll make a lot of users unhappy. You can't pass it to Utf8Decoder.decode unless we change that, and that's the aforementioned breaking change.

It will still not help you if someone imports package:typed_data since Uint8Buffer implements Uint8List anyway, so it's not a solution, just a band-aid on the common case.
That reduces the benefit of the change, which means that the cost/benefit ratio gets worse.

The name should not be ReadOnlySomething because a Uint8List would implement it and not be read-only. Always name interfaces for their positive capabilities, because those must be inherited by subclasses. So, Sequence? ReadList? (bad because it says List, but it is-not a List.) Indexable? (Add Uint8 in front for the typed-data version).

The danger of having such an interface is that users will have to consider, for every API they write, whether to use ReadList or List as parameter type, and as return type. You can always use ReadList as parameter if you don't modify the list, and that's the common case, so most users will be using the (currently) uncommon and longer name. You'll probably use ReadList as return type too, if you are programming defensively, but actually return a List, which is bad because some people will likely just cast the result to List and modify it if that is possible.

Not sure the choice is worth the cost in user mental overhead, and I'm not sure it's worth the technical cost. Dart is an interface based language, and AOT compiling interface based languages is ... well, unprecedented. A small tweak in a corner of the libraries is not going to change that, it'll just introduce more complexity for non-guaranteed efficiency.

All in all, not sold on that idea.

@lrhn
Copy link
Member

lrhn commented Mar 9, 2020

If you make typed data classes unimplementable by third parties, then many things are possible (just not third party implementations). It may not change the design issues, but it does make the change more likely to have a guaranteed positive performance impact, and it makes it "more OK" to have special behavior for those particular classes - for example a read-only superinterface (which will not be assignable to List<int>).

Existing classes implementing typed data types will have to stop doing that. That's only a problem if uses of that code relies on those third-party typed data classes being compatible with the platform types. If, say, a Uint8Buffer is only used internally to build the data, then it doesn't matter whether it implements Uint8List. That is likely the case, and that would be the strategy if we seal platform typed data.

I would also really want to make the unmodifiable lists be backed by actual unmodifiable buffers. If you put a bit on the buffer somehow, and check when creating a Uint8List that the buffer is mutable, and when creating an UnmodifiableUint8List that the buffer is not modifiable, then we have defense in depth - unlike the current hack.
(Since Dart ByteBuffers actually have method, we'd need to actually have an UnmodifiableByteBuffer where asUint8List() returns a Uint8Sequence and ByteBuffer.asUint8List returns a Unit8List. That seems to be type-sound).

@davidmorgan
Copy link
Contributor

/subscribe. Related to Dart and core collection immutability / third party collections:

Sealing or watching List objects: #27755
EfficientLengthIterable for non-SDK libraries: #29862
Statically tracked shared immutable objects: dart-lang/language#125

@ravenblackx
Copy link

Also related
const typed data: #6378

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. type-performance Issue relates to performance or code size
Projects
None yet
Development

No branches or pull requests

6 participants