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
[cfe/ffi] Incremental compiler and FFI transforms #45899
Comments
In the below mind you that my understanding of what the ffi transformation does is somewhat limited. I personally think it would be helpful to (at least) think about the transformation as doing three separate things:
As for the 3 maps mentioned in the first post, 1 and 2 should be filled in when gathering information (or done lazily, if possible) and 3 shouldn't be needed at all I think. I'm not sure I understand why there should be a need to store data in the ast. |
Plan with @jensjoha:
Test cases:
Things that don't need fixing:
|
Run like ``` out/ReleaseX64/dart --enable-asserts pkg/front_end/test/incremental_suite.dart -DupdateExpectations=true -- incremental/crash_05 ``` Crashes like this: ``` NoSuchMethodError: The getter 'iterator' was called on null. Receiver: null Tried calling: iterator #0 Object.noSuchMethod (dart:core-patch/object_patch.dart:54:5) #1 computeStrongComponents.recursivelySearch (package:kernel/util/graph.dart:36:30) #2 computeStrongComponents.recursivelySearch (package:kernel/util/graph.dart:41:26) #3 computeStrongComponents (package:kernel/util/graph.dart:74:24) #4 _FfiDefinitionTransformer.manualVisitInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:131:33) #5 transformLibraries (package:vm/transformations/ffi_definitions.dart:89:15) #6 VmTarget.performModularTransformationsOnLibraries (package:vm/target/vm.dart:162:34) #7 KernelTarget.runBuildTransformations (package:front_end/src/fasta/kernel/kernel_target.dart:1236:19) #8 KernelTarget.buildComponent.<anonymous closure> (package:front_end/src/fasta/kernel/kernel_target.dart:372:7) ``` Run like ``` out/ReleaseX64/dart --enable-asserts pkg/front_end/test/incremental_suite.dart -DupdateExpectations=true -- incremental/crash_06 ``` Crashes like this: ``` Class((whatnot)) not found in compoundCache #0 new NativeTypeCfe (package:vm/transformations/ffi_definitions.dart:775:9) #1 _FfiDefinitionTransformer._replaceFields (package:vm/transformations/ffi_definitions.dart:494:16) #2 _FfiDefinitionTransformer.visitClassInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:198:28) #3 _FfiDefinitionTransformer.manualVisitInTopologicalOrder.<anonymous closure> (package:vm/transformations/ffi_definitions.dart:158:9) #4 List.forEach (dart:core-patch/growable_array.dart:403:8) #5 _FfiDefinitionTransformer.manualVisitInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:134:25) #6 transformLibraries (package:vm/transformations/ffi_definitions.dart:89:15) #7 VmTarget.performModularTransformationsOnLibraries (package:vm/target/vm.dart:162:34) ``` Bug: #45899 Change-Id: I9d42ed16577a79c80f1c3bd77ee82a135d4e107c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197740 Commit-Queue: Jens Johansen <jensj@google.com> Reviewed-by: Daco Harkes <dacoharkes@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>
After https://dart-review.googlesource.com/c/sdk/+/197740, I realized we also don't need the other data structure anymore. In https://dart-review.googlesource.com/c/sdk/+/180189, we disallow empty `Struct`s on the definition sites, making a check on the use sites superfluous. This removes the last dependency between the ffi definitions and ffi use sites transformers. Bug: #45899 TEST=tests/ffi/vmspecific_static_checks_test.dart Change-Id: I6eb5a26d4ece713107ba2a9e6bf601a6e029baa7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198047 Reviewed-by: Clement Skau <cskau@google.com> Commit-Queue: Daco Harkes <dacoharkes@google.com>
A possible future thing to investigate is what happens if we move the size/position calculation to runtime. That way only actually changed libraries would have to be updated (and the special casing in the incremental compiler could be avoided). |
Because FFI inlindes contained Structs, changing for instance the ordering of one class can change the size and position data in another class. We thus have to disable experimental invalidation when the changed class is an FFI class (e.g. a Struct). At least for now that's done crudely by bailing if a changed library explicitly or implicitly (via imports of libraries that export) dart:ffi. http://dartbug.com/45899 Bug: #45899 Change-Id: I2a72df6dace025511f3bbe3a816a8728b713a5e5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198045 Commit-Queue: Jens Johansen <jensj@google.com> Reviewed-by: Daco Harkes <dacoharkes@google.com> Reviewed-by: Johnni Winther <johnniwinther@google.com>
The compounds transformation converting fields into getters and setters now retains the annotations on the getters so that they can be read during recompilation. This splits up `_replaceFields` into `_findFields` and `_replaceFields`. `_findFields` works for both transformed and non-transformed compounds. This splits up `_compoundClassDependencies` out from `_checkFieldAnnotations`. The former is run on all compounds transitively reached from the compounds being compiled, the latter only on the compounds being (re)compiled. `manualVisitInTopologicalOrder` now visits compounds from all libraries, not just the ones in the libraries being (re)compiled. It is responsible for filling the `compoundCache` in topological order. And, this CL introduces the `InvalidNativeTypeCfe` to support processing compounds with invalid fields, which might be nested later in other compounds. Bug: #45899 TEST=pkg/front_end/testcases/incremental/crash_05.yaml TEST=tests/ffi(_2)/* Change-Id: I07a2214fd460f4d5e6a84df81e8b140dd80401dc Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try Fixed: 45899 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/198281 Commit-Queue: Daco Harkes <dacoharkes@google.com> Reviewed-by: Jens Johansen <jensj@google.com>
The FfiTransformer and it's subclasses rely on internal state:
Map<Class, Set<Class>> compoundClassDependencies
builds a dependency graph between classes. If a struct in a library that is updated nests a struct in a library that is not updated, this graph is incomplete (reproduction).Map<Class, CompoundNativeTypeCfe> compoundCache
contains the computed sizes of structs already processed for when processing structs that nest them. If a struct in a library that is updated nests a struct in a library that is not updated, this information is missing and we cannot compute the size of the struct in the updated library.Map<Field, Procedure> replacedGetters
is passed from the definitions transformer to the use site transformer. However, if we have an incremental compilation, and a definition is in a non-updated library, and the use is in an updated library, the use sites might not be replaced. (Or this one was fixed already in #39840, because an updated outline is stored?)These are some examples of multiple data structures being used which have incomplete data on incremental compilation:
sdk/pkg/vm/lib/transformations/ffi_definitions.dart
Lines 108 to 114 in 79331c7
sdk/pkg/vm/lib/transformations/ffi.dart
Lines 775 to 783 in 79331c7
cc @jensjoha @stefantsov @johnniwinther
Two ideas which @stefantsov and I discussed earlier:
The text was updated successfully, but these errors were encountered: