[Breaking Change Request] Deprecate UnmodifiableUint8ListView and friends

[rakudrama]

TL;DR: To ensure that typed data has reliable performance on the JavaScript platforms we need to get rid of the UnmodifiableUint8ListView and similar classes.

What?

• Deprecate UnmodifiableUint8ListView and everything else in unmodifiable_typed_data.dart.

• Replace the constructor for UnmodifiableUint8ListView with a new factory constructor Uint8List.unmodifiableView(Uint8List list), and likewise for the other unmodifiable views.

Why?

Uint8List is sealed to prevent poor performance, but the SDK itself is constructed with the pattern that sealing is trying to prevent - multiple implementations with non-aligned 'shapes', forcing each byte access to be a dispatched aka virtual call. The remedy is to remove the user-visible classes that have this bad pattern.

Benchmarking shows that the polymorphism costs ~10x performance on code that is capable of reading from a regular Uint8List and an UnmodifiableUit8ListView. We could recover that performance by eliminating the polymorphism.
(Comparing golem Benchmarks?benchmark=TypedDataPoly#benchmarks%3DTypedDataPoly.A_UV.array.100%2CTypedDataPoly.A_V.array.100%3Btargets%3Ddart2js-production)

In JavaScript there is a single class that can support Uint8List efficiently: Uint8Array. Having a separate UnmodifiableUint8ListView class makes the operations polymorphic and too slow - often over an order of magnitude slower. Reliable performance of typed data requires all instances of Uint8List to be implemented as Uint8Array.

It is too difficult to provide a fiction that one implementation class (JavaScript Uint8Array) conditionally implements an interface. So we need to have a single interface type in the SDK.

It might turn out that it is possible to use JavaScript subtyping by extending Uint8Array, but in the past we have found that ES6 and later features do not always work efficiently when used in their full generality. Preliminary experiments show that the problem we are trying to avoid with this breaking change request will re-assert itself at a lower level in at least one popular JavaScript engine. Removing UnmodifiableUint8ListView as a user-visible type gives us the freedom to choose the implementation strategy that works well across browsers, including possibly ignoring the checks

Impact

There would be no impact on performance the native VM implementations. (The VM implementations have already been optimized to align the private classes that implement UnmodifiableUint8ListView with those that implement writable Uint8Lists.)

Current usage of the form UnmodifiableUint8ListView(list) becomes Uint8List.unmodifiableView(list).

The unmodifiable views are fairly special purpose and only lightly used so this update should not be a large burden and can be explained in the deprecation message.

What is lost is the ability to test for unmodifiability via x is UnmodifiableUint8ListView. I have not found examples of this. If it is necessary to test for modifiability then an affordance should be provided that is not based on type tests.

How?

• Replace the constructor for UnmodifiableUint8ListView with a new factory constructor Uint8List.unmodifiableView(Uint8List list), and likewise for the other unmodifiable views. The initial implementation of the factory constructors calls the existing private implementation classes.

• Deprecate all the unmodifiable typed data view classes.

• The JavaScript platforms can now change the unmodifiable variant to be a Uint8Array (possibly with a 'permission' tag, similar to how JavaScript's Array implements growable, fixed-length and constant Lists, or some other trick). This will be

• Remove the deprecated classes.

• It might be nice to add immutable: optional arguments to existing constructors. https://dart-review.googlesource.com/c/sdk/+/262241 is an attempt at this which is blocked because it makes the type of many Uint8Lists polymorphic. This will be less of an issue once the implementation type is monomorphic, although some compiler work will still be required to optimize writes.

When?

New factory constructors and deprecation should happen as soon as possible since it gates the other work.

Who?

@rakudrama

[mkustermann]

TL;DR: To ensure that typed data has reliable performance on the JavaScript platforms we need to get rid of the UnmodifiableUint8ListView and similar classes.

Does dart2js have similar performance issues with Unmodifiable{List,Set,Map}View?

It might be nice to add immutable: optional arguments to existing constructors. https://dart-review.googlesource.com/c/sdk/+/262241 is an attempt at this which is blocked because it makes the type of many Uint8Lists polymorphic. This will be less of an issue once the implementation type is monomorphic, although some compiler work will still be required to optimize writes.

Very happy to hear that we may be able to move forward with cl/262241 😄

[lrhn]

The argument for introducing UnmodifiableUint8ListView and the rest, were that some native platforms were presenting views of actually unmodifiable memory, and wanted to not cause the process to crash if it tried to write to unmodifiable memory.
Even if the view is only skin-deep, it was enough to protect users against themselves.

That use-case should still be supported, which means we need a way to create an unmodifiable view on an existing ByteBuffer, not just from a list of numbers.
For consistency, that means adding an asUnmodifiableUint8List(...) method on ByteBuffer, which is the way we create views on byte-buffers. Or adding an {bool unmodifiable = false} parameter to ByteBuffer.asUint8List - which we can't since it has optional positional parameters already.
(We can also add constructors like factory Uint8List.unmodifiableView(ByteBuffer buffer, [int? start, int? length]), but they should delegate to methods on ByteBuffer. Or I guess that design choice isn't as important any more, now that nobody else can implement ByteBuffer, so maybe we can get away with just having the constructors, and not expose how they work.)

Also, when you extract the buffer of an UnmodifiableUint8ListView (using .buffer) you get a special UnmodifiableByteBufferView which always creates unmodifaible views when you use its methods, like asUint8List.
We need to keep that property as well, even if we don't expose a separate UnmodifiableByteBufferView interface.
(Or deprecate and remove the methods on ByteBuffer, again now that nobody else can implement it, we don't have to provide a way for Uint8List.view(ByteBuffer,...) to work with user-created ByteBuffers.)

Optimially, that might just be one bit of information stored on the ByteBuffer, which makes its asUint8List create an unmodifiable view if the flag is set.
But if you can make an unmodifiable view on a modifiable buffer, then we actually do need to return a wrapper of that buffer from an unmodifiable Uint8List's .buffer, because we (probably) don't want to make the existing buffer unmodifiable by setting a bit on it.

That suggests another approach:

• Every ByteBuffer is either modifiable or not, which is a fixed choice made at creation time.
• Each view respects that (and can cache the information at view-creation time, since it's stable, perheaps even using a different implementation class for mutable and immutable views, if that's easier).
• Creating a new unmodifiable Uint8List.unmodifiableFromList(source) will create a new unmodifiable buffer.
• You cannot create an unmodifiable view on a modifiable buffer. (That's potentially breaking. Or rather, it removes a capability, but making the buffer modifiable isn't breking if it wasn't modified today, which it isn't since that would throw an error.)

Then the native use-case for an external-unmodifiable-memory backed ByteBuffer would just create an unmodifiable ByteBuffer first.

(The VM, which still has a non-view Uint8List, would have to have a way to mark those unmodifiable as well. They are effectively the underlying memory that the .buffer exposes.)

[rakudrama]

TL;DR: To ensure that typed data has reliable performance on the JavaScript platforms we need to get rid of the UnmodifiableUint8ListView and similar classes.

Does dart2js have similar performance issues with Unmodifiable{List,Set,Map}View?

Yes for UnmodifiableListView, no for Set and Map.

For the receiver type List we need an interceptor-dispatch, where a[i] compiles to getInterceptor(a).$index(a, i). If the receiver is known to be an javaScript Array, this becomes JSArray_methods.$index(a, i) which is lowered it to a[boundsCheck(i, a.length)] and then we ignore the bounds check at -O4. If the receiver is known to be some class defined in Dart, getInterceptor(a) == a, so the access becomes a.$index(0, i) and possibly inlined.
But with both we have the two-level dispatch.

Less of a problem for Set and Map since both the collection and view are implemented as Dart objects, and often polymorphic (e.g. LinkedHashMap factory), so m[k] compiles to m.$index(0, k) which JavaScript may or may not detect is monomorphic.

It might be nice to add immutable: optional arguments to existing constructors. https://dart-review.googlesource.com/c/sdk/+/262241 is an attempt at this which is blocked because it makes the type of many Uint8Lists polymorphic. This will be less of an issue once the implementation type is monomorphic, although some compiler work will still be required to optimize writes.

Very happy to hear that we may be able to move forward with cl/262241 😄

[rakudrama]

Optimally, that might just be one bit of information stored on the ByteBuffer, which makes its asUint8List create an unmodifiable view if the flag is set. But if you can make an unmodifiable view on a modifiable buffer, then we actually do need to return a wrapper of that buffer from an unmodifiable Uint8List's .buffer, because we (probably) don't want to make the existing buffer unmodifiable by setting a bit on it.

I am happy for now to make .buffer be polymorphic between the native ByteBuffer and an internal _UnmodifiableByteBufferView wrapper that carries the bit (or always use a wrapper, but we might not be able to do that).
Although the polymorphism problem is the same as for the view, it is incurred much less frequently - all you can really do with a buffer is create a view and then access the bytes, and that will be fast.

That suggests another approach:

• Every ByteBuffer is either modifiable or not, which is a fixed choice made at creation time.
• Each view respects that (and can cache the information at view-creation time, since it's stable, perhaps even using a different implementation class for mutable and immutable views, if that's easier).
• Creating a new unmodifiable Uint8List.unmodifiableFromList(source) will create a new unmodifiable buffer.
• You cannot create an unmodifiable view on a modifiable buffer. (That's potentially breaking. Or rather, it removes a capability, but making the buffer modifiable isn't breking if it wasn't modified today, which it isn't since that would throw an error.)

After protecting against SIGSEGV, this might be the most important use-case - handing off a view of data that was assembled via mutation. I'm not sure how, without additional magic, to implement Uint8List.fromLists(lists, immutable: true) with this constraint.

Then the native use-case for an external-unmodifiable-memory backed ByteBuffer would just create an unmodifiable ByteBuffer first.

(The VM, which still has a non-view Uint8List, would have to have a way to mark those unmodifiable as well. They are effectively the underlying memory that the .buffer exposes.)

[rakudrama]

I'm thinking that adding an instance method might be better than another factory constructor.
The problem with a factory constructor called Uint8List.unmodifiableView(someUint8List) is that there already two factory constructors with 'view' in the name.

abstract final class Uint8List implements List<int>, _TypedIntList {
  external factory Uint8List(int length);
  external factory Uint8List.fromList(List<int> elements);

  /// Creates a [Uint8List] _view_ of the specified region in [buffer]....
  factory Uint8List.view(ByteBuffer buffer,
      [int offsetInBytes = 0, int? length]) ...

  /// Creates a [Uint8List] view on a range of elements of [data]....
  factory Uint8List.sublistView(TypedData data, [int start = 0, int? end]) { ... }

  /// Option 1: another factory constructor with 'view' in the name.
  external factory Uint8List.unmodifiableView(Uint8List list);

  /// Option2: an instance method.
  Uint8List asUnmodifiableView();

[itsjustkevin]

@vsmenon @Hixie @grouma could you take a look at this breaking change request?

[grouma]

I can't find any usage in ACX.

[Hixie]

fine by me. fyi @yjbanov

[lrhn]

IIRC, the original request was from the assistant team. That's where we should look for existing uses.

[itsjustkevin]

@vsmenon I am going to approve this change. Please remove the label if you see fit.

[vsmenon]

lgtm

[dcharkes]

What is lost is the ability to test for unmodifiability via x is UnmodifiableUint8ListView. I have not found examples of this. If it is necessary to test for modifiability then an affordance should be provided that is not based on type tests.

We might have a use case: Marking object graphs deeply immutable to be able to share them across isolates in an isolate group.

• [vm/ffi] TypedData, Pointer, multiple isolates, and releasing native resources #54885 (comment)
• Shared Memory Multithreading language#3531 (comment)

For now, I'll just omit support for unmodifiable typed datas in my prototype.

[gmpassos]

We need a isUnmodifiableView

[rakudrama]

@gmpassos could you describe why you need isUnmodifiableView?
If you had isUnmodifiableView, how would you use use it?

[gmpassos]

@rakudrama thanks for the question.

Note that I advocate for having two getters: isUnmodifiable and isUnmodifiableView.

For me, it's very odd to have a method that transforms X into Y but can't check if X is in the Y state.

Here are two classes that demonstrate two basic usages for them:

class ComputationCache {
  final Map<String, Uint8List> _cachedEntries = {};

  void put(String key, Uint8List bs) {
    // The ideal implementation should throw an exception if the
    // [bs] parameter is not unmodifiable, to ensure that it's
    // used correctly and maintain the cache's integrity.
    //
    // if (!bs.isUnmodifiable) throw ArgumentError("Can't store a modifiable buffer");

    // An `isUnmodifiableView` check could avoid a call to `asUnmodifiableView`:
    _cachedEntries[key] = bs.asUnmodifiableView();

    // NOTE: `asUnmodifiableView` lacks documentation on avoiding
    // unnecessary instantiations and wrapping.
  }

  Uint8List? get(String key) {
    var cached = _cachedEntries[key];
    // Since there's no `Unmodifiable` or `UnmodifiableView` type,
    // an assert should inform the rule and check it in tests:
    //
    // assert(cached == null || cached.isUnmodifiableView);

    // Since it's only stored as an UnmodifiableView,
    // it can be returned without a copy, ensuring cache integrity:
    return cached;
  }
}

class BuffersPool {
  final List<Uint8List> _pool = [];

  void add(Uint8List buffer) {
    // The ideal implementation should throw an error if the
    // [buffer] parameter is unmodifiable:
    //
    // if (buffer.isUnmodifiable) throw ArgumentError("Can't store an unmodifiable buffer in the pool.");

    // It should also throw an error if it's an UnmodifiableView:
    // if (buffer.isUnmodifiableView) throw ArgumentError("Can't store an UnmodifiableView of a buffer in the pool");

    _pool.add(buffer);
  }

  Uint8List? get() {
    if (_pool.isNotEmpty) {
      return _pool.removeLast();
    }

    return null;
  }
}

[rakudrama]

So, if buffer.asUnmodifiableView() returned itself, i.e., buffer, if it was already an unmodifiable view, would that satisfy your needs?

[gmpassos]

So, if buffer.asUnmodifiableView() returned itself—i.e., buffer—if it was already an unmodifiable view, would that satisfy your needs?

asUnmodifiableView exists as a way to guarantee performance for each type implementation. IMHO, the contract for asUnmodifiableView should ensure that if it's already unmodifiable, it returns itself (the same instance).

However, this is insufficient for the second example, BuffersPool, where we want to ensure that it is NOT unmodifiable, as a pool should only store modifiable buffers.

Also, how do you write tests to ensure that an implementation of an interface follows a contract where a returned value should be unmodifiable, without attempting to modify it and checking for an error?

[rakudrama]

It is unfortunate that we can't have two interfaces, The breaking change is motivated by a >10x performance penalty for having two separate interfaces on some platforms (when compiling to JavaScript, it requires a very complicated dispatch to do a[i]=v when a can be a TypedArray or also something that is not indexable in JavaScript, like a separate unmodifiable wrapper class).

Ordinary Lists in Dart also suffer from of having no way to test their fixed-length or unmodifiable properties.
So being unable to do tests with typed-data lists that you can't do with ordinary lists anyway is just more of the same problem, not a whole new problem.

[gmpassos]

Well, I think that removing the UnmodifiableView classes can facilitate implementations with better performance. However, I believe that isUnmodifiable and isUnmodifiableView are very important (regardless of the difficulty in implementing them).

[andresgd7]

Go to your tensor.dart (\AppData\Local\Pub\Cache\hosted\pub.dev\tflite_flutter-0.10.4\lib\src\tensor.dart) and change:

  /// Underlying data buffer as bytes.
  Uint8List get data {
    final data = cast<Uint8>(tfliteBinding.TfLiteTensorData(_tensor));
    return UnmodifiableUint8ListView(
        data.asTypedList(tfliteBinding.TfLiteTensorByteSize(_tensor)));
  }

For:

 /// Underlying data buffer as bytes.
  Uint8List get data {
    final data = cast<Uint8>(tfliteBinding.TfLiteTensorData(_tensor));
    return data
        .asTypedList(tfliteBinding.TfLiteTensorByteSize(_tensor))
        .asUnmodifiableView();
  }

[paulobreim]

To solve this problem, I did the following:
flutter pub upgrade --major-versions
With this, the libs that you do not use in pubspec.yaml, but that are causing this problem, such as win32, etc. will be updated and your app will no longer give the UnmodifiableUint8ListView error

[okohub]

Go to your tensor.dart (\AppData\Local\Pub\Cache\hosted\pub.dev\tflite_flutter-0.10.4\lib\src\tensor.dart) and change:

  /// Underlying data buffer as bytes.
  Uint8List get data {
    final data = cast<Uint8>(tfliteBinding.TfLiteTensorData(_tensor));
    return UnmodifiableUint8ListView(
        data.asTypedList(tfliteBinding.TfLiteTensorByteSize(_tensor)));
  }

For:

 /// Underlying data buffer as bytes.
  Uint8List get data {
    final data = cast<Uint8>(tfliteBinding.TfLiteTensorData(_tensor));
    return data
        .asTypedList(tfliteBinding.TfLiteTensorByteSize(_tensor))
        .asUnmodifiableView();
  }

this is somehow black magic and works.