[dart2js] Fix timing issue in protobuf conditional impact enqueueing.
Prior to this fix, the "processedMembers" field on the ResolutionWorldBuilder was getting used as the signal that a member was active and therefore any conditional uses dependent on it should be active.
In some cases the following would occur:
1) The listener gets a signal a member is active. It checks if there are any conditional uses for that member and sees none.
2) A conditional use is registered and the member is not indicated as active yet so the use is stored as pending in ResolutionEnqueuerListener.
3) "processedMembers" in ResolutionWorldBuilder is updated for a member that would mark the conditional use active. This doesn't trigger a check of the pending conditional uses.
If the enqueues happen in this order (which is not always the case), the conditional impact will not be marked active and the field will be erroneously tree shaken.
The solution here is to remove step 3 and add an analogous "usedMembers" update in step 1. Step 1 will always occur before step 2 so the conditional impact ledger and the usedMembers ledger will always stay in sync.
Change-Id: Ib732a4fe5dcc8fc7cbe9a6d37e1c239e287595ff
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/395740
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Nate Biggs <natebiggs@google.com>
diff --git a/pkg/compiler/lib/src/enqueue.dart b/pkg/compiler/lib/src/enqueue.dart
index 8faad63..3b4bf45 100644
--- a/pkg/compiler/lib/src/enqueue.dart
+++ b/pkg/compiler/lib/src/enqueue.dart
@@ -40,10 +40,10 @@
/// backend specific [WorldImpact] of this is returned.
WorldImpact registerUsedElement(MemberEntity member);
- /// Called to register that [use] is a pending condition.
- /// [ConditionalUse.originalConditions] must only contain members that are not known
- /// to be live yet.
- void registerPendingConditionalUse(ConditionalUse use);
+ /// Called to register a new conditional impact, [use]. If the conditional
+ /// impact's condition is satisfied, the returned [WorldImpact] will contain
+ /// the implied impact.
+ WorldImpact registerConditionalUse(ConditionalUse use);
/// Called to register that [value] is statically known to be used. Any
/// backend specific [WorldImpact] of this is returned.
diff --git a/pkg/compiler/lib/src/js_backend/codegen_listener.dart b/pkg/compiler/lib/src/js_backend/codegen_listener.dart
index 4399403..8fe9788f 100644
--- a/pkg/compiler/lib/src/js_backend/codegen_listener.dart
+++ b/pkg/compiler/lib/src/js_backend/codegen_listener.dart
@@ -357,7 +357,7 @@
}
@override
- void registerPendingConditionalUse(ConditionalUse use) {
+ WorldImpact registerConditionalUse(ConditionalUse use) {
throw UnsupportedError(
'Codegen enqueuer does not support conditional impacts.');
}
diff --git a/pkg/compiler/lib/src/js_backend/resolution_listener.dart b/pkg/compiler/lib/src/js_backend/resolution_listener.dart
index d22207e..42ea093 100644
--- a/pkg/compiler/lib/src/js_backend/resolution_listener.dart
+++ b/pkg/compiler/lib/src/js_backend/resolution_listener.dart
@@ -18,11 +18,11 @@
import '../universe/use.dart' show ConditionalUse, StaticUse, TypeUse;
import '../universe/world_impact.dart'
show WorldImpact, WorldImpactBuilder, WorldImpactBuilderImpl;
-import 'field_analysis.dart';
import 'backend_impact.dart';
import 'backend_usage.dart';
import 'checked_mode_helpers.dart';
import 'custom_elements_analysis.dart';
+import 'field_analysis.dart';
import 'interceptor_data.dart';
import 'native_data.dart' show NativeBasicData;
import 'no_such_method_registry.dart';
@@ -258,10 +258,13 @@
return impactBuilder;
}
+ final Set<MemberEntity> _usedMembers = {};
+
@override
WorldImpact registerUsedElement(MemberEntity member) {
WorldImpactBuilderImpl worldImpact = WorldImpactBuilderImpl();
_customElementsAnalysis.registerStaticUse(member);
+ _usedMembers.add(member);
final conditionalUses = _pendingConditionalUses.remove(member);
if (conditionalUses != null) {
// Apply any newly satisfied conditional impacts and remove the condition
@@ -501,9 +504,16 @@
}
@override
- void registerPendingConditionalUse(ConditionalUse use) {
- for (final condition in use.originalConditions) {
- (_pendingConditionalUses[condition] ??= {}).add(use);
+ WorldImpact registerConditionalUse(ConditionalUse use) {
+ WorldImpactBuilderImpl impactBuilder = WorldImpactBuilderImpl();
+ if (use.originalConditions.any(_usedMembers.contains)) {
+ // If any condition is already satisfied, apply the impact immediately.
+ impactBuilder.addImpact(use.impact);
+ } else {
+ for (final condition in use.originalConditions) {
+ (_pendingConditionalUses[condition] ??= {}).add(use);
+ }
}
+ return impactBuilder;
}
}
diff --git a/pkg/compiler/lib/src/resolution/enqueuer.dart b/pkg/compiler/lib/src/resolution/enqueuer.dart
index ba68798..e752a5d 100644
--- a/pkg/compiler/lib/src/resolution/enqueuer.dart
+++ b/pkg/compiler/lib/src/resolution/enqueuer.dart
@@ -8,10 +8,11 @@
import '../common/elements.dart' show ElementEnvironment;
import '../common/tasks.dart' show CompilerTask;
import '../common/work.dart' show WorkItem;
-import '../enqueue.dart';
import '../elements/entities.dart';
import '../elements/types.dart';
+import '../enqueue.dart';
import '../js_backend/annotations.dart';
+import '../js_backend/resolution_listener.dart';
import '../universe/member_usage.dart';
import '../universe/resolution_world_builder.dart' show ResolutionWorldBuilder;
import '../universe/use.dart'
@@ -32,7 +33,7 @@
final CompilerTask task;
final String name;
@override
- final EnqueuerListener listener;
+ final ResolutionEnqueuerListener listener;
final Set<ClassEntity> _recentClasses = Setlet<ClassEntity>();
bool _recentConstants = false;
@@ -337,14 +338,6 @@
@override
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.originalConditions.any(worldBuilder.isMemberProcessed)) {
- applyImpact(conditionalUse.impact);
- } else {
- listener.registerPendingConditionalUse(conditionalUse);
- }
+ applyImpact(listener.registerConditionalUse(conditionalUse));
}
}
