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

dart: Return dart:typed_data equivalent where possible #8289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NotTsunami
Copy link

@NotTsunami NotTsunami commented Apr 22, 2024

Stems from discussion in #8183. This should allow users to be able to access a Uint8List directly without copying over from a list, thus actually offering zero-copy access (for this class, at least) in both the lazy path and the non-lazy path. This is pretty important for signal processing and image processing where we want to avoid any unnecessary list copies.

@github-actions github-actions bot added the dart label Apr 22, 2024
@jamesderlin
Copy link

jamesderlin commented Apr 22, 2024

ByteBuffer has an asInt8List method along with asUint8List, so for symmetry perhaps a Int8List _asInt8List(int offset, int length) method could be added Buffeto rContext, and _FbInt8List could be removed?

@NotTsunami
Copy link
Author

NotTsunami commented Apr 22, 2024

ByteBuffer has an asInt8List method along with asUint8List, so for symmetry perhaps a Int8List _asInt8List(int offset, int length) method could be added Buffeto rContext, and _FbInt8List could be removed?

I'm not opposed to that, but I'd like to see some insight from one of the Dart maintainers to see if this aligns with their view for the Dart side. Ideally, we could expand the same to Uint16ListReader to use their more space-efficient dart:typed_data alternatives (assuming those two follow the same pattern of being subtypes of List<int>), but then the question arises should those classes also offer lazy/greedy paths?

@NotTsunami NotTsunami marked this pull request as ready for review April 23, 2024 14:14
@NotTsunami
Copy link
Author

NotTsunami commented Apr 23, 2024

Ready for review. I tested using @tompark's example code, with the latest version of flatbuffers on pub.dev as well as this fork, and this fork prints the expected bundle1.image1 is already a Uint8List, len=29149 while the latest version on pub.dev does not. The only difference in my test path is that I manually executed the steps from setup.sh instead of writing a script. All tests are passing as well.

@NotTsunami
Copy link
Author

I can provide a test repo if you'd like as well, although setting up Tom's example code also takes <10 minutes, assuming you have Flutter and Python already installed.

@NotTsunami NotTsunami changed the title dart: Always return Uint8List for Uint8ListReader dart: Return dart:typed_data equivalent where possible Apr 23, 2024
* Remove _FbUint8List, _FbInt8List and _FbUint16List to return typed_data types
* Add test for Int8List
* Test Int8List, Uint8List and Uint16List for type equivalence
* Remove unnecessary cast in _writeUTFString to silence linting warning
@NotTsunami
Copy link
Author

I squashed the changes and removed a change related to my other PR to prevent conflicts between the two.

@vaind Hi Ivan, is there any way you might be able to spare some bandwidth in the relatively near future to help review and potentially land both of my open PRs? Cheers.

bdero added a commit to bdero/flutter_scene that referenced this pull request May 15, 2024
Copy link
Contributor

@vaind vaind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. It looks pretty good to me, although there need to be some changes to make the code correct on Big endian machines (I know these are basically non-existent, but since Dart supports them, we should too).

@@ -953,7 +961,7 @@ class Int32Reader extends Reader<int> {
int read(BufferContext bc, int offset) => bc._getInt32(offset);
}

/// The reader of signed 32-bit integers.
/// The reader of signed 16-bit integers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

Comment on lines +1177 to +1180
final listOffset = bc.derefObject(offset);
final length = bc._getUint32(listOffset);

return bc._asUint16List(listOffset + _sizeofUint32, length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works on little-endian hosts. While the vast majority would be little-endian, which is what FlatBuffers uses, it's possible to run Dart on big-endian.

How about we kept the original private classes as a fallback?

Suggested change
final listOffset = bc.derefObject(offset);
final length = bc._getUint32(listOffset);
return bc._asUint16List(listOffset + _sizeofUint32, length);
if (Endian.host == Endian.little) {
final listOffset = bc.derefObject(offset);
final length = bc._getUint32(listOffset);
return bc._asUint16List(listOffset + _sizeofUint32, length);
} else {
_FbUint16List(bc, bc.derefObject(offset));
}

Comment on lines +50 to +51
Uint16List _asUint16List(int offset, int length) =>
_buffer.buffer.asUint16List(_buffer.offsetInBytes + offset, length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the other comment below, this ony has a valid result if running on little-endian platforms.

Suggested change
Uint16List _asUint16List(int offset, int length) =>
_buffer.buffer.asUint16List(_buffer.offsetInBytes + offset, length);
Uint16List _asUint16List(int offset, int length) {
assert(Endian.host == Endian.little);
_buffer.buffer.asUint16List(_buffer.offsetInBytes + offset, length);
}

@NotTsunami
Copy link
Author

Thanks a lot. It looks pretty good to me, although there need to be some changes to make the code correct on Big endian machines (I know these are basically non-existent, but since Dart supports them, we should too).

I think I'd rather add a separate path or built-in for big-endian, otherwise the newly added conditions to the tests will cause the tests to fail on big-endian, as the old classes do not return Uint8List, Int8List, Uint16List, etc on the lazy path. I think the only supported test environment (theoretically) would be ARM on Linux running in big-endian mode? I'll try and set up a qemu environment to test.

@vaind
Copy link
Contributor

vaind commented May 18, 2024

Thanks a lot. It looks pretty good to me, although there need to be some changes to make the code correct on Big endian machines (I know these are basically non-existent, but since Dart supports them, we should too).

I think I'd rather add a separate path or built-in for big-endian, otherwise the newly added conditions to the tests will cause the tests to fail on big-endian, as the old classes do not return Uint8List, Int8List, Uint16List, etc on the lazy path. I think the only supported test environment (theoretically) would be ARM on Linux running in big-endian mode? I'll try and set up a qemu environment to test.

I think it's OK to have different actual return type on big endian, the only contract is that it is compatible with List, everything else is "just" an optimization. It's OK for tests to assert the return value is the expected type based on endianness though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants