Skip to content

Commit

Permalink
Reland "[dart2wasm] Do not assign class ids to anonymous mixin classes."
Browse files Browse the repository at this point in the history 
No changes from original CL. Refactoring in [0] will make us
no longer trigger a bug with this CL where a target that is
only reachable via super calls got the wrong representation.

[0] https://dart-review.googlesource.com/c/sdk/+/375280

Issue flutter/flutter#151473

Original CL description:

  Anonymous mixin application classes get created as a desguaring of mixin
  applications.

  For example this

     mixin M {
       <M-body>
     }
     class Base {}
     class A extends Base with M {}

  gets desugared into

     class Base {}
     abstract class A&Base&M1 extends Base implements M /*isAnonymousMixin*/ {
       <copy-of-M-body>
     }
     class A extends A&Base&M1 {}

  Those anonymous mixin application classes are not real classes: They cannot
  be named in code, cannot be used as type arguments, type literals, type
  checks etc,

  => This CL avoids assigning class ids to them.
  => This will shrink the type name array.
  => This will shrink RTT information.
  => This makes other classes get lower ids assigned

  Size changes of `...opt.wasm` file:
  * Hello: -5%
  * FluteComplex: -1.5%

  Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374320
Change-Id: I09fdc66ba9c2b5deb998988093e859361124f517
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/375103
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
  • Loading branch information
@mkustermann
mkustermann authored and Commit Queue committed Jul 11, 2024
1 parent 0889fdd commit 48599f5
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 53 deletions.
80 changes: 55 additions & 25 deletions pkg/dart2wasm/lib/class_info.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,24 @@ class FieldIndex {
/// by `Object._objectHashCode` wich updates the field first time it's read.
const int initialIdentityHash = 0;

/// We do not assign real class ids to anonymous mixin classes.
const int anonymousMixinClassId = -1;

/// Information about the Wasm representation for a class.
class ClassInfo {
/// The Dart class that this info corresponds to. The top type does not have
/// an associated Dart class.
final Class? cls;

/// The Class ID of this class, stored in every instance of the class.
final int classId;
int get classId {
if (_classId == anonymousMixinClassId) {
throw 'Tried to access class ID of anonymous mixin $cls';
}
return _classId;
}

final int _classId;

/// Depth of this class in the Wasm type hierarchy.
final int depth;
Expand Down Expand Up @@ -169,7 +179,7 @@ class ClassInfo {
w.RefType typeWithNullability(bool nullable) =>
nullable ? nullableType : nonNullableType;

ClassInfo(this.cls, this.classId, this.depth, this.struct, this.superInfo,
ClassInfo(this.cls, this._classId, this.depth, this.struct, this.superInfo,
{this.typeParameterMatch = const {}})
: nullableType = w.RefType.def(struct, nullable: true),
nonNullableType = w.RefType.def(struct, nullable: false);
Expand Down Expand Up @@ -335,9 +345,11 @@ class ClassInfoCollector {
typeParameterMatch: typeParameterMatch);
}
translator.classesSupersFirst.add(info);
translator.classes[classId] = info;
translator.classInfo[cls] = info;
translator.classForHeapType.putIfAbsent(info.struct, () => info!);
if (classId != anonymousMixinClassId) {
translator.classes[classId] = info;
}
}

void _createStructForRecordClass(Map<Class, int> classIds, Class cls) {
Expand Down Expand Up @@ -438,7 +450,8 @@ class ClassInfoCollector {

// Class infos by class-id, will be populated by the calls to
// [_createStructForClass] and [_createStructForRecordClass] below.
translator.classes = List<ClassInfo>.filled(1 + dfsOrder.length, topInfo);
translator.classes =
List<ClassInfo>.filled(classIdNumbering.maxClassId + 1, topInfo);

// Class infos in different order: Infos of super class and super interfaces
// before own info.
Expand Down Expand Up @@ -480,8 +493,7 @@ class ClassInfoCollector {
representation = upperBound(representation, current);
}
}
final classId = classIdNumbering.classIds[cls]!;
final info = translator.classes[classId];
final info = translator.classInfo[cls]!;
info._repr = representation ?? info;
}

Expand All @@ -503,19 +515,23 @@ class ClassInfoCollector {
class ClassIdNumbering {
final Map<Class, List<Class>> _subclasses;
final Map<Class, List<Class>> _implementors;
final List<Range> _concreteSubclassIdRanges;
final Map<Class, Range> _concreteSubclassIdRange;
final Set<Class> _masqueraded;

final List<Class> dfsOrder;
final Map<Class, int> classIds;
final int maxConcreteClassId;
final int maxClassId;

ClassIdNumbering._(
this._subclasses,
this._implementors,
this._concreteSubclassIdRanges,
this._concreteSubclassIdRange,
this._masqueraded,
this.dfsOrder,
this.classIds);
this.classIds,
this.maxConcreteClassId,
this.maxClassId);

final Map<Class, Set<Class>> _transitiveImplementors = {};
Set<Class> _getTransitiveImplementors(Class klass) {
Expand Down Expand Up @@ -550,10 +566,10 @@ class ClassIdNumbering {

ranges = [];
final transitiveImplementors = _getTransitiveImplementors(klass);
final range = _concreteSubclassIdRanges[classIds[klass]!];
final range = _concreteSubclassIdRange[klass]!;
if (!range.isEmpty) ranges.add(range);
for (final implementor in transitiveImplementors) {
final range = _concreteSubclassIdRanges[classIds[implementor]!];
final range = _concreteSubclassIdRange[implementor]!;
if (!range.isEmpty) ranges.add(range);
}
ranges.normalize();
Expand All @@ -578,11 +594,19 @@ class ClassIdNumbering {
late final Class root;
final subclasses = <Class, List<Class>>{};
final implementors = <Class, List<Class>>{};
int abstractClassCount = 0;
int concreteClassCount = 0;
int abstractClassCount = 0;
int anonymousMixinClassCount = 0;
for (final library in translator.component.libraries) {
for (final cls in library.classes) {
cls.isAbstract ? abstractClassCount++ : concreteClassCount++;
if (cls.isAnonymousMixin) {
assert(cls.isAbstract);
anonymousMixinClassCount++;
} else if (cls.isAbstract) {
abstractClassCount++;
} else {
concreteClassCount++;
}
final superClass = cls.superclass;
if (superClass == null) {
root = cls;
Expand All @@ -594,7 +618,6 @@ class ClassIdNumbering {
}
}
}
final int classCount = abstractClassCount + concreteClassCount;

// We have a preference in which order we explore the direct subclasses of
// `Object` as that allows us to keep class ids of certain hierarchies
Expand Down Expand Up @@ -658,14 +681,16 @@ class ClassIdNumbering {

// Maps any class to a dense range of concrete class ids that are subclasses
// of that class.
final concreteSubclassRanges = List<Range>.filled(
firstClassId + classCount, Range.empty(),
growable: false);
final concreteSubclassRange = <Class, Range>{};

int nextConcreteClassId = firstClassId;
int nextAbstractClassId = firstClassId + concreteClassCount;
dfs(root, (Class cls) {
dfsOrder.add(cls);
if (cls.isAnonymousMixin) {
classIds[cls] = anonymousMixinClassId;
return nextConcreteClassId;
}
if (cls.isAbstract) {
var classId = classIds[cls];
if (classId == null) classIds[cls] = nextAbstractClassId++;
Expand All @@ -677,16 +702,21 @@ class ClassIdNumbering {
return nextConcreteClassId - 1;
}, (Class cls, int firstClassId) {
final range = Range(firstClassId, nextConcreteClassId - 1);
if (!cls.isAbstract) {
assert(classIds[cls] == firstClassId);
concreteSubclassRanges[firstClassId] = range;
} else {
concreteSubclassRanges[classIds[cls]!] = range;
}
concreteSubclassRange[cls] = range;
});

return ClassIdNumbering._(subclasses, implementors, concreteSubclassRanges,
masqueraded, dfsOrder, classIds);
assert(dfsOrder.length ==
(concreteClassCount + abstractClassCount + anonymousMixinClassCount));

return ClassIdNumbering._(
subclasses,
implementors,
concreteSubclassRange,
masqueraded,
dfsOrder,
classIds,
firstClassId + concreteClassCount - 1,
firstClassId + concreteClassCount + abstractClassCount - 1);
}
}

Expand Down
64 changes: 36 additions & 28 deletions pkg/dart2wasm/lib/dispatch_table.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class SelectorInfo {
/// Does this method have any tear-off uses?
bool hasTearOffUses = false;

/// Maps class IDs to the selector's member in the class. The member can be
/// abstract.
/// Maps all concrete classes implementing this selector to the relevent
/// implementation member reference for the class.
final Map<int, Reference> targets = {};
late final Set<Reference> targetSet = targets.values.toSet();

Expand All @@ -54,7 +54,7 @@ class SelectorInfo {

/// IDs of classes that implement the member. This does not include abstract
/// classes.
late final List<int> classIds;
final List<int> classIds = [];

/// Number of non-abstract references in [targets].
late final int targetCount;
Expand Down Expand Up @@ -171,6 +171,13 @@ class SelectorInfo {
}

w.ValueType _upperBound(Set<w.ValueType> types, {required bool ensureBoxed}) {
if (types.isEmpty) {
// This happens if the selector doesn't have any targets. Any call site of
// such a selector is unreachable. Though such call sites still have to
// evaluate receiver and arguments. Doing so requires the signature. So we
// create a dummy signature with top types.
return translator.topInfo.nullableType;
}
if (!ensureBoxed && types.length == 1 && types.single.isPrimitive) {
// Unboxed primitive.
return types.single;
Expand Down Expand Up @@ -309,25 +316,24 @@ class DispatchTable {
void build() {
// Collect class/selector combinations

// Maps class IDs to selector IDs of the class
List<Set<int>> selectorsInClass =
List.filled(translator.classes.length, const {});
// Maps class to selector IDs of the class
final selectorsInClass = <Class, Map<SelectorInfo, Reference>>{};

// Add classes to selector targets for their members
for (ClassInfo info in translator.classesSupersFirst) {
Set<int> selectorIds = {};
final ClassInfo? superInfo = info.superInfo;
final Class cls = info.cls ?? translator.coreTypes.objectClass;
final Map<SelectorInfo, Reference> selectors;

// Add the class to its inherited members' selectors. Skip `_WasmBase`:
// it's defined as a Dart class (in `dart._wasm` library) but it's special
// and does not inherit from `Object`.
if (superInfo != null && info.cls != translator.wasmTypesBaseClass) {
int superId = superInfo.classId;
selectorIds = Set.of(selectorsInClass[superId]);
for (int selectorId in selectorIds) {
SelectorInfo selector = _selectorInfo[selectorId]!;
selector.targets[info.classId] = selector.targets[superId]!;
}
final ClassInfo? superInfo = info.superInfo;
if (superInfo == null || cls == translator.wasmTypesBaseClass) {
selectors = {};
} else {
final Class superCls =
superInfo.cls ?? translator.coreTypes.objectClass;
selectors = Map.of(selectorsInClass[superCls]!);
}

/// Add a method (or getter, setter) of the current class ([info]) to
Expand All @@ -342,19 +348,17 @@ class DispatchTable {
SelectorInfo selector = _createSelectorForTarget(reference);
if (reference.asMember.isAbstract) {
// Reference is abstract, do not override inherited concrete member
selector.targets[info.classId] ??= reference;
selectors[selector] ??= reference;
} else {
// Reference is concrete, override inherited member
selector.targets[info.classId] = reference;
selectors[selector] = reference;
}
selectorIds.add(selector.id);
}

// Add the class to its non-static members' selectors. If `info.cls` is
// `null`, that means [info] represents the `#Top` type, which is not a
// Dart class but has the members of `Object`.
for (Member member
in info.cls?.members ?? translator.coreTypes.objectClass.members) {
for (Member member in cls.members) {
// Skip static members
if (!member.isInstanceMember) {
continue;
Expand All @@ -372,19 +376,23 @@ class DispatchTable {
}
}
}
selectorsInClass[cls] = selectors;
}

selectorsInClass[info.classId] = selectorIds;
final maxConcreteClassId = translator.classIdNumbering.maxConcreteClassId;
for (int classId = 0; classId < maxConcreteClassId; ++classId) {
final cls = translator.classes[classId].cls;
if (cls != null) {
selectorsInClass[cls]!.forEach((selectorInfo, target) {
selectorInfo.targets[classId] = target;
selectorInfo.classIds.add(classId);
});
}
}

// Build lists of class IDs and count targets
for (SelectorInfo selector in _selectorInfo.values) {
selector.classIds = selector.targets.keys
.where((classId) =>
!(translator.classes[classId].cls?.isAbstract ?? true))
.toList()
..sort();
Set<Reference> targets =
selector.targets.values.where((t) => !t.asMember.isAbstract).toSet();
final Set<Reference> targets = selector.targets.values.toSet();
selector.targetCount = targets.length;
selector.singularTarget = targets.length == 1 ? targets.single : null;
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/dart2wasm/lib/types.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -914,6 +914,7 @@ class RuntimeTypeInformation {
continue;
}
Class superclass = superclassInfo.cls!;
assert(!superclass.isAnonymousMixin);

// TODO(joshualitt): This includes abstract types that can't be
// instantiated, but might be needed for subtype checks. The majority of
Expand All @@ -925,6 +926,8 @@ class RuntimeTypeInformation {
Iterable<InterfaceType> subtypes = subclasses.map(
(Class cls) => cls.getThisType(coreTypes, Nullability.nonNullable));
for (InterfaceType subtype in subtypes) {
if (subtype.classNode.isAnonymousMixin) continue;

types.interfaceTypeEnvironment._add(subtype);

final List<DartType>? typeArguments = translator.hierarchy
Expand Down

0 comments on commit 48599f5

Please sign in to comment.