Remove static fields in FfiNative's #47625
Labels
area-vm
Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
library-ffi
type-performance
Issue relates to performance or code size
Comments
mkustermann
added
area-vm
7 tasks
|
Not caching addresses in Dart fields would also enable us to think about supporting hot-reload for Implementation pointer: https://dart-review.googlesource.com/c/sdk/+/264842/39/runtime/vm/isolate_reload.cc#2407 |
dcharkes
changed the title
copybara-service bot
pushed a commit
that referenced
this issue
Feb 24, 2023
We only tested a handful of native signatures for `@Native`s and `@FfiNative`s. This CL duplicates the generated FFI call tests which cover a lot of signatures to also run for `@Native`s. Bug: #47625 Change-Id: I475f028c79e13470c2ea4e24040209a915631779 Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-nnbd-win-release-x64-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-nnbd-mac-debug-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-precomp-nnbd-mac-release-arm64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-precomp-ffi-qemu-linux-release-riscv64-try,vm-precomp-ffi-qemu-linux-release-arm-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/284901 Reviewed-by: Martin Kustermann <[email protected]>
copybara-service bot
pushed a commit
that referenced
this issue
Mar 2, 2023
The benchmarks contain some duplicated code, but we want to avoid starting to measure indirection and the compiler not inlining in these benchmarks, so it's better to keep them as plain as possible. Bug: #47625 Change-Id: I7fedd31ce6df83bde3931835ae4e493a252d25b6 Cq-Include-Trybots: luci.dart.try:benchmark-linux-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/284340 Auto-Submit: Daco Harkes <[email protected]> Reviewed-by: Martin Kustermann <[email protected]> Commit-Queue: Daco Harkes <[email protected]> Commit-Queue: Martin Kustermann <[email protected]>
copybara-service bot
pushed a commit
that referenced
this issue
Oct 30, 2023
In a follow up, we will remove the transform from `@Native` to `_asFunctionInternal`. However, we need to ensure that we keep emitting `_nativeEffect` for struct return values classes if `@Native`s are the only place instances are created. The follow up CL will change the expect files. TEST=pkg/vm/test/transformations/ffi_test Bug: #47625 Change-Id: I1685683f48f1d16d6d10606f541f13e17d8583fe Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332800 Reviewed-by: Alexander Markov <[email protected]> Commit-Queue: Alexander Markov <[email protected]> Commit-Queue: Daco Harkes <[email protected]> Auto-Submit: Daco Harkes <[email protected]>
copybara-service bot
pushed a commit
that referenced
this issue
Nov 3, 2023
This reverts commit e16bb21. Reason for revert: Indication this caused engine test failures of the kind: ``` �[0;32m[ RUN ] �[mEmbedderA11yTest.A11yTreeIsConsistentUsingV1Callbacks ../../flutter/shell/platform/embedder/tests/embedder_a11y_unittests.cc:639: Failure Expected equality of these values: std::strncmp(kTooltip, node->tooltip, sizeof(kTooltip) - 1) Which is: 116 0 ``` Original change's description: > [vm/ffi] Optimize `@Native` calls > > This CL removes static fields for storing the `@Native`'s function > addresses. Instead, the function addresses are stored in the object > pool for all archs except for ia32. ia32 has no AOT and no AppJit > snapshots, so the addresses are directly embedded in the code. > > This CL removes the closure wrapping for `@Native`s. Instead of > `pointer.asFunctionInternal()()` where `asFunction` returns a closure > which contains the trampoline, the function is compiled to a body > which contains the trampoline `Native()`. This is possible for > `@Native`s because the dylib and symbol names are known statically. > > Doing the compilation in kernel_to_il instead of a CFE transform > enables supporting static linking later. (The alternative would have > been to transform in the cfe to a `@pragma('vm:cachable-idempotent')` > instead of constructing the IL in kernel_to_il. > > To enable running resolution in ia32 in kernel_to_il.cc, the > resolution function has been made available via > `runtime/lib/ffi_dynamic_library.h`. > > Because the new calls are simply static calls, the TFA can figure > out const arguments flowing to these calls. This leads to constant > locations in the parameters to FfiCalls. So, this CL also introduces > logic to move constants into `NativeLocation`s. > > TEST=runtime/vm/compiler/backend/il_test.cc > TEST=tests/ffi/function_*_native_(leaf_)test.dart > TEST=pkg/vm/testcases/transformations/ffi/ffinative_compound_return.dart > > Closes: #47625 > Closes: #51618 > Change-Id: Ic5d017005dedcedea40c455c4d24dbe774f91603 > CoreLibraryReviewExempt: Internal FFI implementation changes > Cq-Include-Trybots: luci.dart.try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-win-debug-arm64-try,vm-aot-win-debug-x64c-try,vm-aot-win-release-x64-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-fuchsia-release-x64-try,vm-kernel-linux-debug-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-ubsan-linux-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try,vm-win-release-ia32-try > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/284300 > Commit-Queue: Daco Harkes <[email protected]> > Reviewed-by: Alexander Markov <[email protected]> > Reviewed-by: Martin Kustermann <[email protected]> Change-Id: Icc87a6ca33bffecabb15c6b168a06ccc38c2fe5b Cq-Include-Trybots: luci.dart.try:vm-aot-android-release-arm64c-try,vm-aot-android-release-arm_x64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-aot-obfuscate-linux-release-x64-try,vm-aot-win-debug-arm64-try,vm-aot-win-debug-x64c-try,vm-aot-win-release-x64-try,vm-appjit-linux-debug-x64-try,vm-asan-linux-release-x64-try,vm-checked-mac-release-arm64-try,vm-eager-optimization-linux-release-ia32-try,vm-eager-optimization-linux-release-x64-try,vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-ffi-qemu-linux-release-riscv64-try,vm-fuchsia-release-x64-try,vm-kernel-linux-debug-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-msan-linux-release-x64-try,vm-reload-linux-debug-x64-try,vm-reload-rollback-linux-debug-x64-try,vm-ubsan-linux-release-x64-try,vm-win-debug-arm64-try,vm-win-debug-x64-try,vm-win-debug-x64c-try,vm-win-release-ia32-try No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/333840 Bot-Commit: Rubber Stamper <[email protected]> Reviewed-by: Daco Harkes <[email protected]> Commit-Queue: Martin Kustermann <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Right now the FFI kernel transformer lowers
@FfiNatives into static fields containing a lazily populated closure that is created by calling a resolution function.Old natives do not require a static field, so this is in some ways a regression when switching from old natives to ffi natives.
We'd like to avoid usage of static fields for several reasons: It increases the root set in a way that scales as
O(num-isolates x num-ffi-natives).The text was updated successfully, but these errors were encountered: