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.

Sign up for GitHub

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

Jump to bottom

[ffi] NativeCallable.isolateLocal with top level function tearOff should do static call #53096

Open
dcharkes opened this issue Aug 2, 2023 · 2 comments
Open

[ffi] NativeCallable.isolateLocal with top level function tearOff should do static call #53096

dcharkes opened this issue Aug 2, 2023 · 2 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-performance Issue relates to performance or code size

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Aug 2, 2023

With the new NativeCallable.isolateLocal introduced in #52689 and https://dart-review.googlesource.com/c/sdk/+/317060, all callbacks have closure calls.

It might be worth investigating how to turn usages of that API in which tear-offs of toplevel functions are passed in could use static calls. Either through having smart enough machinery in the VM optimizations (not sure if that is feasible), or a CFE transform (definitely feasible, but would mean more machinery).

We could then also consider implementing Pointer.fromFunction in terms of the new NativeCallable.isolateLocal (which just don't have their close method called by users ever).
And we should then also consider whether we should deprecate Pointer.fromFunction.
Unless I'm missing something why we should keep Pointer.fromFunction.
@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Aug 2, 2023
copybara-service bot pushed a commit that referenced this issue Aug 25, 2023
@liamappelbe
[vm/ffi] Closure callbacks for sync callbacks
e8d7425 
Bug: #52689
Change-Id: I54be397cfbf8519fe5b5a51b793fe46d602124d9
Fixes: #52689
Bug: #53096
TEST=isolate_local_function_callbacks_test.dart, plus generated tests and additions to existing tests
CoreLibraryReviewExempt: The isolate and FFI packages are VM-only
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/317060
Commit-Queue: Liam Appelbe <[email protected]>
Reviewed-by: Liam Appelbe <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>
@a-siva
Copy link
Contributor

a-siva commented Nov 16, 2023

@liamappelbe does your change fix this issue too ?

@a-siva a-siva added the triaged Issue has been triaged by sub team label Nov 16, 2023
@liamappelbe
Copy link
Contributor

No, this is a proposed optimisation that hasn't been implemented yet.

@a-siva a-siva added P3 A lower priority bug or feature request type-performance Issue relates to performance or code size labels Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P3 A lower priority bug or feature request triaged Issue has been triaged by sub team type-performance Issue relates to performance or code size
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dcharkes @liamappelbe @a-siva