From aead7438634e7a9cf8effb1fb17e8a4a3d41e859 Mon Sep 17 00:00:00 2001 From: Nate Biggs Date: Tue, 13 Feb 2024 20:18:01 +0000 Subject: [PATCH] [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 Commit-Queue: Nate Biggs --- .../src/deferred_load/entity_data_info.dart | 18 ++++++++--- pkg/compiler/lib/src/enqueue.dart | 15 ++++------ pkg/compiler/lib/src/ir/impact.dart | 5 ++-- pkg/compiler/lib/src/ir/impact_data.dart | 30 ++++++++++++------- pkg/compiler/lib/src/ir/protobuf_impacts.dart | 14 +++++---- .../lib/src/js_backend/codegen_listener.dart | 3 +- pkg/compiler/lib/src/js_backend/enqueuer.dart | 3 +- .../src/js_backend/resolution_listener.dart | 14 ++++++--- .../lib/src/kernel/kernel_impact.dart | 15 +++++----- pkg/compiler/lib/src/resolution/enqueuer.dart | 25 +++++++--------- .../universe/resolution_world_builder.dart | 6 ---- pkg/compiler/lib/src/universe/use.dart | 10 +++++-- .../lib/src/universe/world_impact.dart | 26 ++++++++-------- 13 files changed, 99 insertions(+), 85 deletions(-) diff --git a/pkg/compiler/lib/src/deferred_load/entity_data_info.dart b/pkg/compiler/lib/src/deferred_load/entity_data_info.dart index 6013d5f4951..56d30cb503c 100644 --- a/pkg/compiler/lib/src/deferred_load/entity_data_info.dart +++ b/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]. diff --git a/pkg/compiler/lib/src/enqueue.dart b/pkg/compiler/lib/src/enqueue.dart index a627cbf3bdf..3b740c86360 100644 --- a/pkg/compiler/lib/src/enqueue.dart +++ b/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 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> conditionalUses); + void processConditionalUse(ConditionalUse conditionalUse); EnqueuerListener get listener; void open(FunctionEntity? mainMethod, Iterable libraries) { diff --git a/pkg/compiler/lib/src/ir/impact.dart b/pkg/compiler/lib/src/ir/impact.dart index 8275502a007..234c45e6dc9 100644 --- a/pkg/compiler/lib/src/ir/impact.dart +++ b/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); + void registerConditionalImpact(ConditionalImpactData impact); } class ImpactBuilderData { diff --git a/pkg/compiler/lib/src/ir/impact_data.dart b/pkg/compiler/lib/src/ir/impact_data.dart index 208391bdff7..954fd16080f 100644 --- a/pkg/compiler/lib/src/ir/impact_data.dart +++ b/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) { + 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 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>? conditionalImpacts; - ir.TreeNode? conditionalSource; - ir.TreeNode? conditionalReplacement; + List? _conditionalImpacts; // TODO(johnniwinther): Remove these when CFE provides constants. List? _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) { diff --git a/pkg/compiler/lib/src/ir/protobuf_impacts.dart b/pkg/compiler/lib/src/ir/protobuf_impacts.dart index 9dec590f312..5d00cfd008d 100644 --- a/pkg/compiler/lib/src/ir/protobuf_impacts.dart +++ b/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 = []; 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))); } } diff --git a/pkg/compiler/lib/src/js_backend/codegen_listener.dart b/pkg/compiler/lib/src/js_backend/codegen_listener.dart index 78d62c2d92f..4399403b67f 100644 --- a/pkg/compiler/lib/src/js_backend/codegen_listener.dart +++ b/pkg/compiler/lib/src/js_backend/codegen_listener.dart @@ -357,8 +357,7 @@ class CodegenEnqueuerListener extends EnqueuerListener { } @override - void registerPendingConditionalUses( - MemberEntity member, List uses) { + void registerPendingConditionalUse(ConditionalUse use) { throw UnsupportedError( 'Codegen enqueuer does not support conditional impacts.'); } diff --git a/pkg/compiler/lib/src/js_backend/enqueuer.dart b/pkg/compiler/lib/src/js_backend/enqueuer.dart index 775349568f4..811993d6038 100644 --- a/pkg/compiler/lib/src/js_backend/enqueuer.dart +++ b/pkg/compiler/lib/src/js_backend/enqueuer.dart @@ -311,6 +311,5 @@ class CodegenEnqueuer extends Enqueuer { worldBuilder.processedEntities; @override - void processConditionalUses( - Map> conditionalUses) {} + void processConditionalUse(ConditionalUse conditionalUse) {} } diff --git a/pkg/compiler/lib/src/js_backend/resolution_listener.dart b/pkg/compiler/lib/src/js_backend/resolution_listener.dart index fa8b330375f..007b90511e5 100644 --- a/pkg/compiler/lib/src/js_backend/resolution_listener.dart +++ b/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> _pendingConditionalUses = {}; + final Map> _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 uses) { - (_pendingConditionalUses[member] ??= []).addAll(uses); + void registerPendingConditionalUse(ConditionalUse use) { + for (final condition in use.conditions) { + (_pendingConditionalUses[condition] ??= {}).add(use); + } } } diff --git a/pkg/compiler/lib/src/kernel/kernel_impact.dart b/pkg/compiler/lib/src/kernel/kernel_impact.dart index 29d280c046c..176df00b90b 100644 --- a/pkg/compiler/lib/src/kernel/kernel_impact.dart +++ b/pkg/compiler/lib/src/kernel/kernel_impact.dart @@ -820,11 +820,8 @@ class KernelImpactConverter implements ImpactRegistry { } @override - void registerConditionalImpacts( - ir.Member condition, Iterable 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); } } diff --git a/pkg/compiler/lib/src/resolution/enqueuer.dart b/pkg/compiler/lib/src/resolution/enqueuer.dart index f2612d281ad..cbe23f4a986 100644 --- a/pkg/compiler/lib/src/resolution/enqueuer.dart +++ b/pkg/compiler/lib/src/resolution/enqueuer.dart @@ -336,20 +336,15 @@ class ResolutionEnqueuer extends Enqueuer { } @override - void processConditionalUses( - Map> 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); - } - } else { - listener.registerPendingConditionalUses(condition, uses); - } - }); + 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.registerPendingConditionalUse(conditionalUse); + } } } diff --git a/pkg/compiler/lib/src/universe/resolution_world_builder.dart b/pkg/compiler/lib/src/universe/resolution_world_builder.dart index 385a087d259..80837cdc65c 100644 --- a/pkg/compiler/lib/src/universe/resolution_world_builder.dart +++ b/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> populateHierarchyNodes() { Map> typesImplementedBySubclasses = {}; diff --git a/pkg/compiler/lib/src/universe/use.dart b/pkg/compiler/lib/src/universe/use.dart index 320b977aa5d..94fdcfb7b3f 100644 --- a/pkg/compiler/lib/src/universe/use.dart +++ b/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 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); } diff --git a/pkg/compiler/lib/src/universe/world_impact.dart b/pkg/compiler/lib/src/universe/world_impact.dart index 101b8eae55d..642d59f3468 100644 --- a/pkg/compiler/lib/src/universe/world_impact.dart +++ b/pkg/compiler/lib/src/universe/world_impact.dart @@ -39,7 +39,7 @@ class WorldImpact { Iterable get constantUses => const []; - Map> get conditionalUses => const {}; + Iterable get conditionalUses => const []; void _forEach( Iterable 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 conditionalUses); + void registerConditionalUse(ConditionalUse conditionalUse); } class WorldImpactBuilderImpl extends WorldImpactBuilder { @@ -102,7 +104,7 @@ class WorldImpactBuilderImpl extends WorldImpactBuilder { Set? _staticUses; Set? _typeUses; Set? _constantUses; - Map>? _conditionalUses; + List? _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 impacts) { - ((_conditionalUses ??= {})[condition] ??= []).addAll(impacts); + void registerConditionalUse(ConditionalUse conditionalUse) { + (_conditionalUses ??= []).add(conditionalUse); } @override - Map> get conditionalUses { - return _conditionalUses ?? const {}; + Iterable get conditionalUses { + return _conditionalUses ?? const []; } } @@ -249,11 +250,10 @@ class TransformedWorldImpact extends WorldImpactBuilder { } @override - void registerConditionalUses( - MemberEntity condition, Iterable conditionalUses) {} + void registerConditionalUse(ConditionalUse conditionalUse) {} @override - Map> get conditionalUses => const {}; + Iterable get conditionalUses => const []; @override String toString() {