Skip to content

[dart2asm] Improve/fix TFA record field dynamic call metadata #51363

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

Closed
osa1 opened this issue Feb 10, 2023 · 3 comments
Closed

[dart2asm] Improve/fix TFA record field dynamic call metadata #51363

osa1 opened this issue Feb 10, 2023 · 3 comments
Labels
area-dart2wasm Issues for the dart2wasm compiler.

Comments

@osa1
Copy link
Member

osa1 commented Feb 10, 2023

In records implementation (https://dart-review.googlesource.com/c/sdk/+/280461) we currently need to consider every record shape class field as dynamically used: (in dispatch_table.dart)

    // Dynamic call metadata is not accurate for record fields, so we consider
    // them to be dynamically called.
    final isRecordMember =
        member.enclosingClass?.superclass == translator.recordInfo.cls;

    final calledDynamically = !isWasmType &&
            (metadata.getterCalledDynamically ||
                metadata.methodOrSetterCalledDynamically ||
                member.name.text == "call") ||
        isRecordMember;

This is because TFA never considers a record class field as dynamically used. For example, for the record (1, 2, a: 'hi') we generate this class:

@pragma('wasm:entry-point')
class Record_2_a {
  @pragma('wasm:entry-point')
  final Object? $1;

  @pragma('wasm:entry-point')
  final Object? $2;

  @pragma('wasm:entry-point')
  final Object? a;

  ...
}

For members $1, $2, and a, TFA generates ProcedureAttributesMetadatas, but getterCalledDynamically of those metadata are always false.

I don't understand TFA in detail so I'm not sure if this is a bug and/or can be improved, but it's worth investigating.

@osa1 osa1 added the area-dart2wasm Issues for the dart2wasm compiler. label Feb 10, 2023
@alexmarkov
Copy link
Contributor

As far as I understand, currently dart2wasm introduces synthetic classes for records, but doesn't lower any of the record types, record field access, record literals and record constants. So, in a kernel program those synthetic classes are just separate entities, unrelated to how record fields are accessed. As a result, we cannot expect from TFA to magically move inferred information about dynamic accesses to record fields into real fields of those synthetic classes.

As we're going along the path of converting records to classes, it would be much cleaner to do the full lowering of records into classes before TFA. That transformation should replace all record types, record field access, record literals and record constants, in addition to just adding record classes. This will ensure that program is consistent and dynamic access to a record field is just a dynamic access to real field of a synthetic class.

@osa1
Copy link
Member Author

osa1 commented Feb 13, 2023

As far as I understand, currently dart2wasm introduces synthetic classes for records, but doesn't lower any of the record types, record field access, record literals and record constants.

Correct.

So, in a kernel program those synthetic classes are just separate entities, unrelated to how record fields are accessed. As a result, we cannot expect from TFA to magically move inferred information about dynamic accesses to record fields into real fields of those synthetic classes.

These synthetic classes are added as Record subtypes, so they're not completely invisible in kernel.

As we're going along the path of converting records to classes, it would be much cleaner to do the full lowering of records into classes before TFA. That transformation should replace all record types, record field access, record literals and record constants, in addition to just adding record classes. This will ensure that program is consistent and dynamic access to a record field is just a dynamic access to real field of a synthetic class.

I'm working on this, but the transformation is not entirely trivial, because of loops in DartType nodes. (I don't yet understand why/how/where those loops are formed)

@osa1
Copy link
Member Author

osa1 commented Mar 7, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart2wasm Issues for the dart2wasm compiler.
Projects
None yet
Development

No branches or pull requests

2 participants