Skip to content

[vm/ffi] Implement ExternalTypedData unwrapping to Pointers in non-leaf calls #54856

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

Open
dcharkes opened this issue Feb 8, 2024 · 3 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Feb 8, 2024

After #44589, the question becomes what can we do for non-leaf calls.

If the typed-data is an external typed-data, unwrapping it on non-leaf calls is fine.

  • If the typed-data is internal, invoking the FFI call should throw.
  • Similar to leaf-calls: The native code should not take ownership of the pointer.
  • Similar to leaf-calls: There should be a reachability fence on the typed-data arguments if we load the untagged address before the FfiCallInstr.

Possible alternatives for avoiding buffer copies on non-leaf calls.

  • Structure the API with Pointers instead, likely nested in opaque classes that implement Finalizable.
    • This runs into issues if one both wants to give access to this wrapper class which might have a finalizer and a typed-data which might have a finalizer for the same underlying data. Using asTypedList with a finalizer and then views on top of that to ensure there's only a single finalizer runs into the problems again that there's no
  • Enable checking on typed-data if it's external or not, then users can make a copy to native memory if it's not external.
    • The downside here is non-predictable performance characters, sometimes an invocation will copy, and sometimes it wont.
  • Adding the Pointer on the TypedData with an Expando
    • The issue here is that one would need to add a reachability fence manually after the FFI call that used the Pointer

cc @mkustermann @craiglabenz @Piinks

@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Feb 8, 2024
@mkustermann
Copy link
Member

👍 As I've suggested offline, allowing the address-of operator on typed data in leaf & non-leaf calls and make it throw in non-leaf calls if it's not external typed data would be my preferred solution. It avoids flaky behavior where we sometimes copy and sometimes not. It allows it in non-leaf calls. The only requirement is that users have to ensure that their typed data is backed by external memory, which is guaranteed if users e.g. use Pointer.asTypedList() to obtain the typed data. It would work on external TD as well as TD views on external TD.

It does bear the question whether we should have a mechanism to allow users to check whether a typed data is backed by external memory, so users can e.g. write assert(myTypedData.isExternal) when passing it around, as we do not expose a separate ExternalTypedList like interface (/cc @lrhn ). But this isn't a requirement per-se.

@lrhn
Copy link
Member

lrhn commented Feb 8, 2024

I think dart:ffi could introduce a type like abstract interface type ExternalTypedData implements TypedData {} which all its external implementations satisfy. Not sure we want it, because without intersection types, we can't ask for an ExternalTypedData & Uint8List, so people will quickly ask for ExternalUint8List as well. Which is a misnomer, because it's the underlying buffer which is external, we shouldn't need more than one view wrapper.
(If we worry that dart:ffi can extends a final type from dart:typed_data, there are many ways to hack that.)

I'd rather that dart:fffi exposed an extension isExternal getter on TypedData (and on ByteBuffer), which is definitely within normal Dart capabilities.

I don't think the dart:typed_data library should expose any notion of anything being "external". To the plain typed-data user, all typed-data is the same. Only dart:ffi knows that some things are external and other things are not.
(It can also add .isExternal on Strings if it wants to. And isExternalOneByte/isExternalTwoByte on the strings, in case someone wants an Array<Uint8> backed by an external one-byte string's bytes.)

About addressOf, an operation that can fail in some cases, and where the places it can fail are determined syntactically, is hard to prepare for.
It can't fail at compile-time if some backing values work and others do not, so all errors become runtime errors.

Maybe have a .addressOf that can be used everywhere, and always throws at runtime on non-external/heap-backed data, and a .tailAddressOf that can only be used in tail position, but works on all data.
Then it can be a compile-time error to use .tailAddressOf in an non-tail position. That ensures that if your code uses .tailAddressOf and compiles, then it won't throw at runtime.
If you use plain addressOf, you can use .isExternal to guard against throwing.
Together, those two can make it possible for a program to never throw at runtime, even if it doesn't know (initally) whether a value is external, and without silently allowing an intended tail addressOf to a runtime error if it's used in a non-tail position.

The last one is a failure case that worries me. If we say that the same .addressOf can be used on non-external type data, but only in tail position, and on external data everywhere, then we can't tell whether a particular use was intended to be in tail position, and therefore we can't warn if it isn't. And that's how runtime errors happen.

@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 8, 2024

Thanks @mkustermann and @lrhn!

I'd rather that dart:ffi exposed an extension isExternal getter on TypedData (and on ByteBuffer), which is definitely within normal Dart capabilities.

Only dart:ffi knows that some things are external and other things are not. (It can also add .isExternal on Strings if it wants to. And isExternalOneByte/isExternalTwoByte on the strings, in case someone wants an Array<Uint8> backed by an external one-byte string's bytes.)

The last one is a failure case that worries me. If we say that the same .addressOf can be used on non-external type data, but only in tail position, and on external data everywhere, then we can't tell whether a particular use was intended to be in tail position, and therefore we can't warn if it isn't. And that's how runtime errors happen.

I don't really like .tailAddressOf, but this is a convincing argument. Maybe we can come up with better naming. How about addressOfInternal or addressOfDart, to signify that this could be Dart memory instead of external.

(Naming side note: It should probably be .address not .addressOf. If it was a method addressOf(...) would make sense. So .addressPossiblyInternal, .addressInternal, .addressDart, ... ?)

We'll need to come up with some consistent naming strategy involving (a) 'external', (b) 'internal'/'dart', (c) 'possibly internal'/'possibly dart', (d) 'leaf call' and relate all of these concepts in the documentation. .address should probably refer to isExternal in its doc comment etc.

Also, if we start supporting addressOf on TypedData if it's external and we add an bool get isExternal, we should add the same capability to Struct, Union, and Array.

@a-siva a-siva added type-enhancement A request for a change that isn't a bug triaged Issue has been triaged by sub team labels Feb 8, 2024
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. library-ffi triaged Issue has been triaged by sub team type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants