Skip to content

Commit 1841a8d

Browse files
natebiggsCommit Queue
authored andcommitted
[dart2js] Add support for tree shaking protobuf messages with mixins.
Typically these mixins introduce additional reflective features on the messages extending them. This made it unfavorable to tree shake fields on these messages. However, some customers have tooling that makes it so the set of messages/fields accessed reflectively through mixins is statically known. Therefore, they can inject special code to retain those messages/fields and still benefit from treeshaking on the rest of the fields in those messages. This is a separate flag because it introduces new opportunities for breakages that users may want to roll out separately. Testing on an internal build showed an additional ~2.8% code size reduction from this additional tree shaking. Change-Id: I6bbe6209acc26f8e8904c81f6960b71e4b516f25 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/414820 Reviewed-by: Mayank Patke <[email protected]> Commit-Queue: Nate Biggs <[email protected]>
1 parent 97ba123 commit 1841a8d

File tree

4 files changed

+48
-6
lines changed

4 files changed

+48
-6
lines changed

pkg/compiler/lib/src/commandline_options.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ class Flags {
7777
static const String laxRuntimeTypeToString = '--lax-runtime-type-to-string';
7878

7979
static const String enableProtoShaking = '--enable-proto-shaking';
80+
static const String enableProtoMixinShaking = '--enable-proto-mixin-shaking';
8081

8182
static const String platformBinaries = '--platform-binaries=.+';
8283

pkg/compiler/lib/src/dart2js.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,7 @@ Future<api.CompilationResult> compile(
514514
_OneOption(Flags.omitAsCasts, passThrough),
515515
_OneOption(Flags.laxRuntimeTypeToString, passThrough),
516516
_OneOption(Flags.enableProtoShaking, passThrough),
517+
_OneOption(Flags.enableProtoMixinShaking, passThrough),
517518
_OneOption(Flags.benchmarkingProduction, passThrough),
518519
_OneOption(Flags.benchmarkingExperiment, passThrough),
519520
_OneOption(Flags.soundNullSafety, passThrough),

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

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,24 +73,55 @@ import 'impact_data.dart';
7373
class ProtobufImpactHandler implements ConditionalImpactHandler {
7474
static const String protobufLibraryUri = 'package:protobuf/protobuf.dart';
7575

76+
static bool _hasGeneratedMessageSuperclass(
77+
ir.Class protobufGeneratedMessageClass,
78+
ir.Class enclosingClass,
79+
) {
80+
ir.Class? current = enclosingClass;
81+
while (current != null) {
82+
if (current == protobufGeneratedMessageClass) return true;
83+
84+
current = current.superclass;
85+
}
86+
return false;
87+
}
88+
7689
static ProtobufImpactHandler? createIfApplicable(
7790
KernelToElementMap elementMap,
7891
ir.Member node,
7992
) {
8093
if (!elementMap.options.enableProtoShaking) return null;
8194

95+
if (node.name.text != metadataFieldName) return null;
96+
8297
// Not all programs will include the protobuf library. Ideally those
8398
// programs wouldn't enable proto shaking but we can be conservative and not
8499
// assume the library exists.
85100
final protobufGeneratedMessageClass = elementMap.env.libraryIndex
86101
.tryGetClass(protobufLibraryUri, 'GeneratedMessage');
87102

88-
// Only applicable if the member is in a subclass of GeneratedMessage and
89-
// the name matches the static metadata field.
90-
if (protobufGeneratedMessageClass != null &&
91-
node.enclosingClass?.superclass == protobufGeneratedMessageClass &&
92-
node.name.text == metadataFieldName) {
93-
return ProtobufImpactHandler._(elementMap, node.enclosingClass!);
103+
if (protobufGeneratedMessageClass == null) return null;
104+
105+
final enclosingClass = node.enclosingClass;
106+
if (enclosingClass == null) return null;
107+
108+
// Applicable if the member is in a subclass of GeneratedMessage.
109+
if (enclosingClass.superclass == protobufGeneratedMessageClass) {
110+
return ProtobufImpactHandler._(elementMap, enclosingClass);
111+
}
112+
113+
// Applicable if the member is in a subclass of GeneratedMessage with mixins
114+
// included.
115+
if (elementMap.options.enableProtoMixinShaking) {
116+
final superclass = enclosingClass.superclass;
117+
if (superclass != null &&
118+
superclass.isMixinApplication &&
119+
_hasGeneratedMessageSuperclass(
120+
protobufGeneratedMessageClass,
121+
superclass,
122+
)) {
123+
return ProtobufImpactHandler._(elementMap, enclosingClass);
124+
}
94125
}
95126
return null;
96127
}

pkg/compiler/lib/src/options.dart

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,7 @@ class CompilerOptions implements DiagnosticOptions {
705705
bool disableDiagnosticByteCache = false;
706706

707707
bool enableProtoShaking = false;
708+
bool enableProtoMixinShaking = false;
708709

709710
bool get producesModifiedDill =>
710711
stage == CompilerStage.closedWorld && enableProtoShaking;
@@ -959,6 +960,10 @@ class CompilerOptions implements DiagnosticOptions {
959960
Flags.laxRuntimeTypeToString,
960961
)
961962
..enableProtoShaking = _hasOption(options, Flags.enableProtoShaking)
963+
..enableProtoMixinShaking = _hasOption(
964+
options,
965+
Flags.enableProtoMixinShaking,
966+
)
962967
..testMode = _hasOption(options, Flags.testMode)
963968
..trustPrimitives = _hasOption(options, Flags.trustPrimitives)
964969
..useFrequencyNamer =
@@ -1172,6 +1177,10 @@ class CompilerOptions implements DiagnosticOptions {
11721177
mergeFragmentsThreshold = _mergeFragmentsThreshold;
11731178
}
11741179

1180+
if (enableProtoMixinShaking) {
1181+
enableProtoShaking = true;
1182+
}
1183+
11751184
environment['dart.web.assertions_enabled'] = '$enableUserAssertions';
11761185
environment['dart.tool.dart2js'] = '${true}';
11771186
environment['dart.tool.dart2js.minify'] = '$enableMinification';

0 commit comments

Comments
 (0)