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:ffi interaction with dynamic #35902

Closed
dcharkes opened this issue Feb 11, 2019 · 12 comments
Closed

dart:ffi interaction with dynamic #35902

dcharkes opened this issue Feb 11, 2019 · 12 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi part-of:pointer-api
Milestone

Comments

@dcharkes
Copy link
Contributor

While not efficient, dart:ffi should be available with dynamic (except for trampolines).

Pointer<Int8> p = allocate();
dynamic q = p;
q.store(123);
q.free();

TODO: test whether it works and write tests for this functionality.

@dcharkes dcharkes added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Feb 11, 2019
@dcharkes dcharkes added this to the Dart VM FFI Flutter MVP milestone Feb 11, 2019
@dcharkes dcharkes modified the milestones: Dart VM FFI Flutter MVP, Dart VM FFI 1.0 Feb 15, 2019
@mit-mit mit-mit removed this from the Dart VM FFI 1.0 milestone Mar 11, 2019
@sjindel-google
Copy link
Contributor

sjindel-google commented Jun 11, 2019

We will lock out these methods on dynamic for now so that switching to extension methods will be a non-breaking change (against the runtime behaviour).

Note: we need to determine whether "existing" code which passes the type arguments to load will be broken.

@sjindel-google
Copy link
Contributor

This task is mentioned in code which checks that type arguments to load and store are statically known, which is a separate additional issue.

@sjindel-google
Copy link
Contributor

Another thing to check is that the trampolines and other ops that work on Pointers but aren't instance methods still work when the Pointer class is implemented (instead of extended).

@sjindel-google
Copy link
Contributor

@mraleph @dcharkes
This could be another substantial task to work on next: it requires re-writing the dynamic invocation forwarders in the VM.

@sjindel-google
Copy link
Contributor

Not sure what the exact scope of this issue is, but we need to also disallow dynamic invocations of asFunction and lookupFunction.

@dcharkes
Copy link
Contributor Author

Not sure what the exact scope of this issue is, but we need to also disallow dynamic invocations of asFunction and lookupFunction.

That would be trivial with extension methods.

If we ship before extension methods land, we need to prohibit this. However, this is work that we would be obsolete once we switch to extension methods, so I'd give this low priority.

@sjindel-google
Copy link
Contributor

We can give it low priority as long as the dynamic invocations don't make the VM crash.

@sjindel-google
Copy link
Contributor

sjindel-google commented Jul 4, 2019

@dcharkes Assigning myself since suggested that we should remove dynamic invocations of Pointer.load and Pointer.store altogether in https://dart-review.googlesource.com/c/sdk/+/107513

@dcharkes
Copy link
Contributor Author

This is part of the CL that addresses Pointer optimizations (#38172), and is currently in review.

@mkustermann
Copy link
Member

We already track extension methods in #37773 and do not plan support for dynamic load/store atm, right? So can we close this issue?

@sjindel-google
Copy link
Contributor

Yes, this issue is obsolete.

@dcharkes
Copy link
Contributor Author

dcharkes commented Oct 7, 2019

The CL that rules out invocations on dynamic hasn't landed yet. It should land today.

@dcharkes dcharkes reopened this Oct 7, 2019
dart-bot pushed a commit that referenced this issue Oct 7, 2019
This reverts commit 3712ed2.

Reason for revert: Breaks Arm32 precompiled.
Issue: #38737

Original change's description:
> [vm/ffi] Optimize Pointer operations for statically known types
> 
> This CL optimizes Pointer operations in hot loops for Pointer<NativeInteger/NativeDouble/Pointer> (not for structs).
> 
> Design: go/dart-ffi-pointers-il
> 
> It provides roughly a 100x speedup for the FfiMemory benchmark. The next 5x speedup is to get rid of allocations due to `load` and `store` not being inlined.
> 
> FFI API is changed to enable optimizations:
> 
> * Disable dynamic invocations of Pointer.load / Pointer.store.
> * Disallow implicit downcast of argument passed to Pointer.store.
> * Stop zeroing out Pointer.address on Pointer.free().
> 
> Issue: #38172
> 
> Related issues:
> Closes: #35902 (Disallowing dynamic invocations of Pointer ops.)
> Closes: #37385 (Function variance checking.)
> 
> Change-Id: I96058d8b5b49052eb6999f084372e6f08b4f6f17
> Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-dartkb-linux-debug-simarm64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-dartkb-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/117547
> Commit-Queue: Daco Harkes <dacoharkes@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>

TBR=kustermann@google.com,sjindel@google.com,dacoharkes@google.com

Change-Id: I3b7923ace45beaa9f99119e9ea20c1e52b429ad8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Issue: #38172
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try, vm-ffi-android-debug-arm64-try, app-kernel-linux-debug-x64-try, vm-kernel-linux-debug-ia32-try, vm-dartkb-linux-debug-simarm64-try, vm-kernel-win-debug-x64-try, vm-kernel-win-debug-ia32-try, vm-dartkb-linux-debug-x64-try, vm-kernel-precomp-linux-debug-x64-try, vm-dartkb-linux-release-x64-abi-try, vm-kernel-precomp-android-release-arm64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/120582
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Oct 8, 2019
Original CL in patchset 1.
Fix for simdbc in patchset 4.
Fix for arm32 precompiled in: https://dart-review.googlesource.com/c/sdk/+/120660/

This CL optimizes Pointer operations in hot loops for Pointer<NativeInteger/NativeDouble/Pointer> (not for structs).

Design: go/dart-ffi-pointers-il

It provides roughly a 100x speedup for the FfiMemory benchmark. The next 5x speedup is to get rid of allocations due to `load` and `store` not being inlined.

FFI API is changed to enable optimizations:

* Disable dynamic invocations of Pointer.load / Pointer.store.
* Disallow implicit downcast of argument passed to Pointer.store.
* Stop zeroing out Pointer.address on Pointer.free().

Issue: #38172

Related issues:

Closes: #35902 (Disallowing dynamic invocations of Pointer ops.)
Closes: #37385 (Function variance checking)
Change-Id: I3921a595fd05026d6ca565ace496771d7c1d877b
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-dartkb-linux-debug-simarm64-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-dartkb-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-mac-debug-simdbc64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-reload-mac-release-simdbc64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-precomp-mac-release-simarm_x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/120661
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi part-of:pointer-api
Projects
None yet
Development

No branches or pull requests

4 participants