[cfe/ffi] Incremental compiler and FFI transforms #45899

dcharkes · 2021-05-04T07:37:39Z

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

    
           Map<Class, Set<Class>> compoundClassDependencies = {}; 
        
           Map<Class, bool> fieldsValid = {}; 
        
           Map<Class, CompoundNativeTypeCfe> compoundCache = {}; 
        
           Map<Field, Procedure> replacedGetters = {}; 
        
           Map<Field, Procedure> replacedSetters = {}; 
        
           Set<Class> emptyCompounds = {};

sdk/pkg/vm/lib/transformations/ffi.dart

Lines 775 to 783 in 79331c7

    
           /// Contains all information collected by _FfiDefinitionTransformer that is 
        
           /// needed in _FfiUseSiteTransformer. 
        
           class FfiTransformerData { 
        
             final Map<Field, Procedure> replacedGetters; 
        
             final Map<Field, Procedure> replacedSetters; 
        
             final Set<Class> emptyCompounds; 
        
             FfiTransformerData( 
        
                 this.replacedGetters, this.replacedSetters, this.emptyCompounds); 
        
           }

cc @jensjoha @stefantsov @johnniwinther

Two ideas which @stefantsov and I discussed earlier:

Store the information distributed in the AST, for example in annotations, so that it can be looked up again.
A facility to store some kind of meta data in the outline.

The text was updated successfully, but these errors were encountered:

jensjoha · 2021-05-04T07:50:26Z

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:

It gathers data. This might need the full component. Alternatively it could perhaps be calculated lazily. My guess would be that what it needs is only libraries that import ffi (which would quickly throw most libraries away) and only classes that extends Struct (which would quickly throw many classes away) and that thus looking at the entire component is not a big deal.
Transforms new compilation of ffi stuff (extending struct and whatnot). This should only need the new libraries. Old call-sites might theoretically need updating too, but given that fields now have 2 references (one for the getter and one for the setter), creating the new getter and setter correct should make this free and automatic without any patching.
Error reporting. My guess would be that this is doable with the information gathered in step 1 and/or when transforming in step 2.

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.

dcharkes · 2021-05-04T09:15:16Z

Plan with @jensjoha:

Update information gathering to gather from all libraries (class hierarchy) not only the libraries being transformed.
Bail out on not recompiling dependent libraries if FFI structs are involved, same as with mixins (e.g. we're inlining things).
Remove getter/setter replacement (because those references should be updated automatically).
We need tests for the various scenarios.

Test cases:

static final field dart.core::int* #sizeOf of an outer struct should be updated when a nested struct size is updated by adding a field
static final field dart.core::int* #sizeOf of an outer struct should be updated when a nested struct size is updated by a reordering of fields (@Int8(), @Int8(), @Int64() -> @Int8(), @Int64(), @Int8()).
The offsets in generated fields of outer struct should be updated if inner struct in front of it changes size: return dart.ffi::_loadDouble(this.{dart.ffi::_Compound::_typedDataBase}, (#C14).{dart.core::List::[]}(dart.ffi::_abi()));, #C14 should change in ffi_01.yaml.world.1.expect on incremental compilation.

Things that don't need fixing:

Replacing sizeOf<MyStruct>() calls in the AST with get MyStruct.#sizeOf, the reference should automatically be updated when MyStruct is recompiled.

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>

jensjoha · 2021-05-05T11:58:03Z

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>

dcharkes added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. area-front-end Use area-front-end for front end / CFE / kernel format related issues. library-ffi labels May 4, 2021

dcharkes changed the title ~~[cfe/ffi] Incremental compiler and FFI transformes don't play well together~~ [cfe/ffi] Incremental compiler and FFI transforms May 4, 2021

dcharkes closed this as completed May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cfe/ffi] Incremental compiler and FFI transforms #45899

[cfe/ffi] Incremental compiler and FFI transforms #45899

dcharkes commented May 4, 2021 •

edited

jensjoha commented May 4, 2021

dcharkes commented May 4, 2021 •

edited

jensjoha commented May 5, 2021

[cfe/ffi] Incremental compiler and FFI transforms #45899

[cfe/ffi] Incremental compiler and FFI transforms #45899

Comments

dcharkes commented May 4, 2021 • edited

jensjoha commented May 4, 2021

dcharkes commented May 4, 2021 • edited

jensjoha commented May 5, 2021

dcharkes commented May 4, 2021 •

edited

dcharkes commented May 4, 2021 •

edited