[dart2js] Fix protobuf shaking algorithm.

- Reorganize conditional impacts/uses so that unused impacts only overwrite nodes that no other member condition is satisfying.
- Use `isMemberProcessed` instead of `isLiveInstanceMember` since some member conditions might be static members.
- Update deferred load algorithm to account for conditional uses.

Change-Id: I53ce6d283c5b79e0a7c9e7562fdcfe744e37bbc7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/352180
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>

pkg/compiler/lib/src/deferred_load/entity_data_info.dart

@@ -114,7 +114,7 @@ class EntityDataInfoBuilder {
     if (!closedWorld.isMemberUsed(element)) {
       return;
     }
-    _addDependenciesFromImpact(element);
+    _addDependenciesFromEntityImpact(element);
     ConstantCollector.collect(elementMap, element, this);
   }
 
@@ -235,17 +235,27 @@ class EntityDataInfoBuilder {
     }
   }
 
-  /// Extract any dependencies that are known from the impact of [element].
-  void _addDependenciesFromImpact(MemberEntity element) {
-    WorldImpact worldImpact = impactCache[element]!;
+  void _addFromConditionalUse(ConditionalUse conditionalUse) {
+    if (conditionalUse.conditions.any(closedWorld.isMemberUsed)) {
+      _addDependenciesFromImpact(conditionalUse.impact);
+    }
+  }
+
+  void _addDependenciesFromImpact(WorldImpact worldImpact) {
     worldImpact.forEachStaticUse(_addFromStaticUse);
     worldImpact.forEachTypeUse(_addFromTypeUse);
+    worldImpact.forEachConditionalUse((_, use) => _addFromConditionalUse(use));
 
     // TODO(johnniwinther): Use rti need data to skip unneeded type
     // arguments.
     worldImpact.forEachDynamicUse(
         (_, use) => addTypeListDependencies(use.typeArguments));
   }
+
+  /// Extract any dependencies that are known from the impact of [element].
+  void _addDependenciesFromEntityImpact(MemberEntity element) {
+    _addDependenciesFromImpact(impactCache[element]!);
+  }
 }
 
 /// Collects the necessary [EntityDataInfo] for a given [EntityData].

pkg/compiler/lib/src/enqueue.dart

@@ -40,12 +40,10 @@ abstract class EnqueuerListener {
   /// backend specific [WorldImpact] of this is returned.
   WorldImpact registerUsedElement(MemberEntity member);
 
-  /// Called to register that [uses] are conditionally applied if [member] is
-  /// used. [member] should only ever be a pending entity (i.e. it is not known
-  /// to be live yet). Entities that are already known to be live should have
-  /// their impacts applied immediately.
-  void registerPendingConditionalUses(
-      MemberEntity member, List<ConditionalUse> uses);
+  /// Called to register that [use] is a pending condition.
+  /// [ConditionalUse.conditions] must only contain members that are not known
+  /// to be live yet.
+  void registerPendingConditionalUse(ConditionalUse use);
 
   /// Called to register that [value] is statically known to be used. Any
   /// backend specific [WorldImpact] of this is returned.
@@ -102,7 +100,7 @@ abstract class Enqueuer {
     worldImpact.forEachDynamicUse((_, use) => processDynamicUse(use));
     worldImpact.forEachTypeUse(processTypeUse);
     worldImpact.forEachConstantUse((_, use) => processConstantUse(use));
-    processConditionalUses(worldImpact.conditionalUses);
+    worldImpact.forEachConditionalUse((_, use) => processConditionalUse(use));
   }
 
   bool checkNoEnqueuedInvokedInstanceMethods(
@@ -122,8 +120,7 @@ abstract class Enqueuer {
   void processTypeUse(MemberEntity? member, TypeUse typeUse);
   void processDynamicUse(DynamicUse dynamicUse);
   void processConstantUse(ConstantUse constantUse);
-  void processConditionalUses(
-      Map<MemberEntity, List<ConditionalUse>> conditionalUses);
+  void processConditionalUse(ConditionalUse conditionalUse);
   EnqueuerListener get listener;
 
   void open(FunctionEntity? mainMethod, Iterable<Uri> libraries) {

pkg/compiler/lib/src/ir/impact.dart

@@ -7,7 +7,7 @@ import 'package:kernel/type_environment.dart' as ir;
 
 import '../serialization/serialization.dart';
 import 'constants.dart';
-import 'impact_data.dart' show ImpactData;
+import 'impact_data.dart' show ConditionalImpactData, ImpactData;
 import 'runtime_type_analysis.dart';
 
 /// Interface for collecting world impact data.
@@ -178,8 +178,7 @@ abstract class ImpactRegistry {
   void registerExternalProcedureNode(ir.Procedure node);
   void registerForeignStaticInvocationNode(ir.StaticInvocation node);
   void registerConstSymbolConstructorInvocationNode();
-  void registerConditionalImpacts(
-      ir.Member condition, Iterable<ImpactData> impactData);
+  void registerConditionalImpact(ConditionalImpactData impact);
 }
 
 class ImpactBuilderData {

pkg/compiler/lib/src/ir/impact_data.dart

@@ -38,10 +38,11 @@ abstract class ConditionalImpactHandler {
   /// otherwise.
   ImpactData? beforeInstanceInvocation(ir.InstanceInvocation node);
 
-  /// Invoked after children of [node] are analyzed. Takes [currentData] which
-  /// should be the [ImpactData] prior to visiting [node].
+  /// Invoked after children of [node] are analyzed. [registry] allows any extra
+  /// impacts to be registered. This is typically used to register conditional
+  /// impacts in the registry.
   void afterInstanceInvocation(
-      ir.InstanceInvocation node, ImpactData currentData);
+      ir.InstanceInvocation node, ImpactRegistry registry);
 }
 
 class _ConditionalImpactBuilder extends ImpactBuilder {
@@ -58,8 +59,8 @@ class _ConditionalImpactBuilder extends ImpactBuilder {
 
     super.visitInstanceInvocation(node);
 
-    _conditionalHandler.afterInstanceInvocation(node, oldData);
     _data = oldData;
+    _conditionalHandler.afterInstanceInvocation(node, this);
   }
 }
 
@@ -1155,13 +1156,22 @@ class ImpactBuilder extends ir.RecursiveVisitor implements ImpactRegistry {
   }
 
   @override
-  void registerConditionalImpacts(
-      ir.Member condition, Iterable<ImpactData> impactData) {
+  void registerConditionalImpact(ConditionalImpactData impact) {
     // Ensure conditional impact is registered on parent impact, `_data`.
-    ((_data.conditionalImpacts ??= {})[condition] ??= []).addAll(impactData);
+    (_data._conditionalImpacts ??= []).add(impact);
   }
 }
 
+class ConditionalImpactData {
+  final ir.TreeNode? source;
+  final ir.TreeNode? replacement;
+  final List<ir.Member> conditions;
+  final ImpactData impactData;
+
+  ConditionalImpactData(this.conditions, this.impactData,
+      {this.source, this.replacement});
+}
+
 /// Data object that contains the world impact data derived purely from kernel.
 /// It is critical that all of the data in this class be invariant to changes in
 /// the AST that occur after modular compilation and before deserializing the
@@ -1202,9 +1212,7 @@ class ImpactData {
   List<_RecordLiteral>? _recordLiterals;
   List<_RuntimeTypeUse>? _runtimeTypeUses;
   List<_ForInData>? _forInData;
-  Map<ir.Member, List<ImpactData>>? conditionalImpacts;
-  ir.TreeNode? conditionalSource;
-  ir.TreeNode? conditionalReplacement;
+  List<ConditionalImpactData>? _conditionalImpacts;
 
   // TODO(johnniwinther): Remove these when CFE provides constants.
   List<ir.Constructor>? _externalConstructorNodes;
@@ -1649,7 +1657,7 @@ class ImpactData {
       }
     }
 
-    conditionalImpacts?.forEach(registry.registerConditionalImpacts);
+    _conditionalImpacts?.forEach(registry.registerConditionalImpact);
 
     // TODO(johnniwinther): Remove these when CFE provides constants.
     if (_externalConstructorNodes != null) {

pkg/compiler/lib/src/ir/protobuf_impacts.dart

@@ -6,6 +6,7 @@ import 'package:kernel/ast.dart' as ir;
 import 'package:kernel/type_algebra.dart' as ir;
 
 import '../kernel/element_map.dart';
+import 'impact.dart';
 import 'impact_data.dart';
 
 /// Handles conditional impact creation for protobuf metadata.
@@ -165,15 +166,13 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
     // conditional on the associated field being reachable.
     return _impactData = interfaceTarget.enclosingClass == _builderInfoClass &&
             metadataInitializers.contains(node.name.text)
-        ? (ImpactData()
-          ..conditionalSource = node
-          ..conditionalReplacement = _buildProtobufMetadataPlaceholder(node))
+        ? ImpactData()
         : null;
   }
 
   @override
   void afterInstanceInvocation(
-      ir.InstanceInvocation node, ImpactData currentData) {
+      ir.InstanceInvocation node, ImpactRegistry registry) {
     // This instance invocation is not a metadata initializer.
     if (_impactData == null) return;
 
@@ -186,6 +185,7 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
     // Iterate through all the accessors and find ones which are annotated
     // with a matching tag number. These are the accessors that the current
     // metadata initializer is conditional on.
+    final accessors = <ir.Member>[];
     for (final procedure in _messageClass.procedures) {
       for (final annotation in procedure.annotations) {
         final constant = (annotation as ir.ConstantExpression).constant;
@@ -197,11 +197,13 @@ class ProtobufImpactHandler implements ConditionalImpactHandler {
                   .value
                   .toInt();
           if (tagNumber == procedureTagNumber) {
-            ((currentData.conditionalImpacts ??= {})[procedure] ??= [])
-                .add(_impactData!);
+            accessors.add(procedure);
           }
         }
       }
     }
+    registry.registerConditionalImpact(ConditionalImpactData(
+        accessors, _impactData!,
+        source: node, replacement: _buildProtobufMetadataPlaceholder(node)));
   }
 }

pkg/compiler/lib/src/js_backend/codegen_listener.dart

@@ -357,8 +357,7 @@ class CodegenEnqueuerListener extends EnqueuerListener {
   }
 
   @override
-  void registerPendingConditionalUses(
-      MemberEntity member, List<ConditionalUse> uses) {
+  void registerPendingConditionalUse(ConditionalUse use) {
     throw UnsupportedError(
         'Codegen enqueuer does not support conditional impacts.');
   }

pkg/compiler/lib/src/js_backend/enqueuer.dart

@@ -311,6 +311,5 @@ class CodegenEnqueuer extends Enqueuer {
       worldBuilder.processedEntities;
 
   @override
-  void processConditionalUses(
-      Map<MemberEntity, List<ConditionalUse>> conditionalUses) {}
+  void processConditionalUse(ConditionalUse conditionalUse) {}
 }

pkg/compiler/lib/src/js_backend/resolution_listener.dart

@@ -49,7 +49,7 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
   /// Contains conditional uses for members that have not been marked as live
   /// yet. Any entries remaining in here after the queue is finished are
   /// considered unreachable.
-  final Map<MemberEntity, List<ConditionalUse>> _pendingConditionalUses = {};
+  final Map<MemberEntity, Set<ConditionalUse>> _pendingConditionalUses = {};
 
   ResolutionEnqueuerListener(
       this._options,
@@ -262,8 +262,13 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
     _customElementsAnalysis.registerStaticUse(member);
     final conditionalUses = _pendingConditionalUses.remove(member);
     if (conditionalUses != null) {
+      // Apply any newly satisfied conditional impacts and remove the condition
+      // from the pending list of other members.
       for (final conditionalUse in conditionalUses) {
         worldImpact.addImpact(conditionalUse.impact);
+        for (final condition in conditionalUse.conditions) {
+          _pendingConditionalUses[condition]?.remove(conditionalUse);
+        }
       }
     }
 
@@ -494,8 +499,9 @@ class ResolutionEnqueuerListener extends EnqueuerListener {
   }
 
   @override
-  void registerPendingConditionalUses(
-      MemberEntity member, List<ConditionalUse> uses) {
-    (_pendingConditionalUses[member] ??= []).addAll(uses);
+  void registerPendingConditionalUse(ConditionalUse use) {
+    for (final condition in use.conditions) {
+      (_pendingConditionalUses[condition] ??= {}).add(use);
+    }
   }
 }

pkg/compiler/lib/src/kernel/kernel_impact.dart

@@ -820,11 +820,8 @@ class KernelImpactConverter implements ImpactRegistry {
   }
 
   @override
-  void registerConditionalImpacts(
-      ir.Member condition, Iterable<ImpactData> impacts) {
-    final conditionalUses = impacts.map((impactData) => ConditionalUse(
-        source: impactData.conditionalSource,
-        replacement: impactData.conditionalReplacement,
+  void registerConditionalImpact(ConditionalImpactData impact) {
+    final conditionalUse = ConditionalUse(
         // TODO(natebiggs): Make KernelImpactConverter stateless so that we
         // don't need one per impact.
         impact: KernelImpactConverter(
@@ -840,9 +837,11 @@ class KernelImpactConverter implements ImpactRegistry {
                 _customElementsResolutionAnalysis,
                 _rtiNeedBuilder,
                 _annotationsData)
-            .convert(impactData)));
+            .convert(impact.impactData),
+        conditions: impact.conditions.map(elementMap.getMember).toList(),
+        source: impact.source,
+        replacement: impact.replacement);
 
-    impactBuilder.registerConditionalUses(
-        elementMap.getMember(condition), conditionalUses);
+    impactBuilder.registerConditionalUse(conditionalUse);
   }
 }

pkg/compiler/lib/src/resolution/enqueuer.dart

@@ -336,20 +336,15 @@ class ResolutionEnqueuer extends Enqueuer {
   }
 
   @override
-  void processConditionalUses(
-      Map<MemberEntity, List<ConditionalUse>> conditionalUses) {
-    conditionalUses.forEach((condition, uses) {
-      // Only register a conditional use as pending if the condition member is
-      // not live. If it is already live apply the attached impacts immediately.
-      // [worldBuilder.isInstanceMemberLive] checks against the set of members
-      // that have been registered as used by the enqueuer.
-      if (worldBuilder.isInstanceMemberLive(condition)) {
-        for (final use in uses) {
-          applyImpact(use.impact);
-        }
+  void processConditionalUse(ConditionalUse conditionalUse) {
+    // Only register a conditional use as pending if no condition member is
+    // live. If any condition member is live then immediately apply the impact.
+    // `worldBuilder.isMemberProcessed` checks whether the member is used
+    // including all static and instance members.
+    if (conditionalUse.conditions.any(worldBuilder.isMemberProcessed)) {
+      applyImpact(conditionalUse.impact);
     } else {
-        listener.registerPendingConditionalUses(condition, uses);
-      }
-    });
+      listener.registerPendingConditionalUse(conditionalUse);
+    }
   }
 }

pkg/compiler/lib/src/universe/resolution_world_builder.dart

@@ -881,12 +881,6 @@ class ResolutionWorldBuilder extends WorldBuilder implements World {
     }
   }
 
-  bool isInstanceMemberLive(MemberEntity element) {
-    return element.isAbstract
-        ? _liveAbstractInstanceMembers.contains(element)
-        : _liveInstanceMembers.contains(element);
-  }
-
   Map<ClassEntity, Set<ClassEntity>> populateHierarchyNodes() {
     Map<ClassEntity, Set<ClassEntity>> typesImplementedBySubclasses = {};
 

pkg/compiler/lib/src/universe/use.dart

@@ -1053,7 +1053,13 @@ class ConditionalUse {
   final ir.TreeNode? source;
   final ir.TreeNode? replacement;
   final WorldImpact impact;
+  final List<MemberEntity> conditions;
 
-  ConditionalUse({this.source, this.replacement, required this.impact})
-      : assert((source == null) == (replacement == null));
+  ConditionalUse(
+      {this.source,
+      this.replacement,
+      required this.impact,
+      required this.conditions})
+      : assert((source == null) == (replacement == null)),
+        assert(conditions.isNotEmpty);
 }

pkg/compiler/lib/src/universe/world_impact.dart

@@ -39,7 +39,7 @@ class WorldImpact {
 
   Iterable<ConstantUse> get constantUses => const [];
 
-  Map<MemberEntity, List<ConditionalUse>> get conditionalUses => const {};
+  Iterable<ConditionalUse> get conditionalUses => const [];
 
   void _forEach<U>(
           Iterable<U> uses, void Function(MemberEntity?, U) visitUse) =>
@@ -53,6 +53,9 @@ class WorldImpact {
       _forEach(typeUses, visitUse);
   void forEachConstantUse(void Function(MemberEntity?, ConstantUse) visitUse) =>
       _forEach(constantUses, visitUse);
+  void forEachConditionalUse(
+          void Function(MemberEntity?, ConditionalUse) visitUse) =>
+      _forEach(conditionalUses, visitUse);
 
   bool get isEmpty => true;
 
@@ -87,8 +90,7 @@ abstract class WorldImpactBuilder extends WorldImpact {
   void registerTypeUse(TypeUse typeUse);
   void registerStaticUse(StaticUse staticUse);
   void registerConstantUse(ConstantUse constantUse);
-  void registerConditionalUses(
-      MemberEntity condition, Iterable<ConditionalUse> conditionalUses);
+  void registerConditionalUse(ConditionalUse conditionalUse);
 }
 
 class WorldImpactBuilderImpl extends WorldImpactBuilder {
@@ -102,7 +104,7 @@ class WorldImpactBuilderImpl extends WorldImpactBuilder {
   Set<StaticUse>? _staticUses;
   Set<TypeUse>? _typeUses;
   Set<ConstantUse>? _constantUses;
-  Map<MemberEntity, List<ConditionalUse>>? _conditionalUses;
+  List<ConditionalUse>? _conditionalUses;
 
   WorldImpactBuilderImpl([this.member]);
 
@@ -125,7 +127,7 @@ class WorldImpactBuilderImpl extends WorldImpactBuilder {
     impact.staticUses.forEach(registerStaticUse);
     impact.typeUses.forEach(registerTypeUse);
     impact.constantUses.forEach(registerConstantUse);
-    impact.conditionalUses.forEach(registerConditionalUses);
+    impact.conditionalUses.forEach(registerConditionalUse);
   }
 
   @override
@@ -169,14 +171,13 @@ class WorldImpactBuilderImpl extends WorldImpactBuilder {
   }
 
   @override
-  void registerConditionalUses(
-      MemberEntity condition, Iterable<ConditionalUse> impacts) {
-    ((_conditionalUses ??= {})[condition] ??= []).addAll(impacts);
+  void registerConditionalUse(ConditionalUse conditionalUse) {
+    (_conditionalUses ??= []).add(conditionalUse);
   }
 
   @override
-  Map<MemberEntity, List<ConditionalUse>> get conditionalUses {
-    return _conditionalUses ?? const {};
+  Iterable<ConditionalUse> get conditionalUses {
+    return _conditionalUses ?? const [];
   }
 }
 
@@ -249,11 +250,10 @@ class TransformedWorldImpact extends WorldImpactBuilder {
   }
 
   @override
-  void registerConditionalUses(
-      MemberEntity condition, Iterable<ConditionalUse> conditionalUses) {}
+  void registerConditionalUse(ConditionalUse conditionalUse) {}
 
   @override
-  Map<MemberEntity, List<ConditionalUse>> get conditionalUses => const {};
+  Iterable<ConditionalUse> get conditionalUses => const [];
 
   @override
   String toString() {