From 72c3c8ea61689744cc3adb417da82d31acdf06bd Mon Sep 17 00:00:00 2001 From: Sigmund Cherem Date: Sat, 13 Jan 2018 01:42:32 +0000 Subject: [PATCH] Several deferred loading improvements: - fix some discrepancies between old and new deferred loading. * Kernel is now using the correct world impact * For the old FE, I'm more careful about avoiding metadata nodes when mirros are disabled as well. * Better approximate when a constant is deferred in the kernel codegen - make 'dump()' independent of the IR, so we can easily compare them side by side. - exclude primitives in kernel since we don't need to track them. I didn't change the old front end because it is not as easy to do. I did this change because kernel was processing many more constants than the old frontend (the old frontend skipped constants that were never evaluated, we have no way to do it selectively like that in the new front end). Change-Id: I337d3fd818753125476b7390da5d900ebdc02709 Reviewed-on: https://dart-review.googlesource.com/34509 Reviewed-by: Emily Fortuna Commit-Queue: Sigmund Cherem --- pkg/compiler/lib/src/deferred_load.dart | 42 ++++++++++++++++--- .../lib/src/kernel/deferred_load.dart | 8 ++-- .../lib/src/kernel/kernel_strategy.dart | 2 +- .../lib/src/resolution/deferred_load.dart | 13 ++++++ pkg/compiler/lib/src/ssa/builder_kernel.dart | 3 +- .../deferred_loading_test.dart | 6 +-- .../libs/deferred_constant1_lib3.dart | 2 +- .../dont_inline_deferred_constants_lib1.dart | 17 ++++++-- .../dont_inline_deferred_constants_lib2.dart | 2 +- .../dont_inline_deferred_constants_main.dart | 5 ++- 10 files changed, 78 insertions(+), 22 deletions(-) diff --git a/pkg/compiler/lib/src/deferred_load.dart b/pkg/compiler/lib/src/deferred_load.dart index 8ba05a1a3ba..d0996693db1 100644 --- a/pkg/compiler/lib/src/deferred_load.dart +++ b/pkg/compiler/lib/src/deferred_load.dart @@ -95,6 +95,9 @@ abstract class DeferredLoadTask extends CompilerTask { /// Will be `true` if the program contains deferred libraries. bool isProgramSplit = false; + /// Whether mirrors have been used in the program. + bool _isMirrorsUsed = false; + static const ImpactUseCase IMPACT_USE = const ImpactUseCase('Deferred load'); /// A mapping from the name of a defer import to all the output units it @@ -217,7 +220,9 @@ abstract class DeferredLoadTask extends CompilerTask { collectConstantsInBody(analyzableElement, constants); } - collectConstantsFromMetadata(element, constants); + if (_isMirrorsUsed) { + collectConstantsFromMetadata(element, constants); + } if (element is FunctionEntity) { _collectTypeDependencies( @@ -630,6 +635,7 @@ abstract class DeferredLoadTask extends CompilerTask { work() { var queue = new WorkQueue(this.importSets); + _isMirrorsUsed = closedWorld.backendUsage.isMirrorsUsed; // Add `main` and their recursive dependencies to the main output unit. // We do this upfront to avoid wasting time visiting these elements when @@ -650,7 +656,7 @@ abstract class DeferredLoadTask extends CompilerTask { element = element is ClassElement ? element.implementation : element; queue.addElement(element, importSets.mainSet); } - if (closedWorld.backendUsage.isMirrorsUsed) { + if (_isMirrorsUsed) { addMirrorElementsForLibrary(queue, main.library, importSets.mainSet); } @@ -781,14 +787,37 @@ abstract class DeferredLoadTask extends CompilerTask { return "${outName}_$name$extension"; } + bool ignoreEntityInDump(Entity element) => false; + /// Creates a textual representation of the output unit content. String dump() { Map> elementMap = >{}; Map> constantMap = >{}; _elementToSet.forEach((Entity element, ImportSet importSet) { - elementMap.putIfAbsent(importSet.unit, () => []).add('$element'); + if (ignoreEntityInDump(element)) return; + var elements = elementMap.putIfAbsent(importSet.unit, () => []); + var id = element.name ?? '$element'; + if (element is MemberEntity) { + var cls = element.enclosingClass?.name; + if (cls != null) id = '$cls.$id'; + if (element.isSetter) id = '$id='; + id = '$id member'; + } else if (element is ClassEntity) { + id = '$id cls'; + } else if (element is TypedefEntity) { + id = '$id typedef'; + } else if (element is Local) { + var context = (element as dynamic).memberContext.name; + id = element.name == null || element.name == '' ? '' : id; + id = '$context.$id'; + id = '$id local'; + } + elements.add(id); }); _constantToSet.forEach((ConstantValue value, ImportSet importSet) { + // Skip primitive values: they are not stored in the constant tables and + // if they are shared, they end up duplicated anyways across output units. + if (value.isPrimitive) return; constantMap .putIfAbsent(importSet.unit, () => []) .add(value.toStructuredText()); @@ -801,9 +830,10 @@ abstract class DeferredLoadTask extends CompilerTask { unitText.write('
'); } else { unitText.write(' imports:'); - var imports = outputUnit._imports.map((i) => '${i.uri}').toList() - ..sort(); - for (var i in imports) { + var imports = outputUnit._imports + .map((i) => '${i.enclosingLibrary.canonicalUri.resolveUri(i.uri)}') + .toList(); + for (var i in imports..sort()) { unitText.write('\n $i:'); } } diff --git a/pkg/compiler/lib/src/kernel/deferred_load.dart b/pkg/compiler/lib/src/kernel/deferred_load.dart index 0117c71b2dd..f2641c6a24c 100644 --- a/pkg/compiler/lib/src/kernel/deferred_load.dart +++ b/pkg/compiler/lib/src/kernel/deferred_load.dart @@ -145,16 +145,16 @@ class ConstantCollector extends ir.RecursiveVisitor { constants.add(elementMap.getConstantValue(node)); @override - void visitIntLiteral(ir.IntLiteral literal) => add(literal); + void visitIntLiteral(ir.IntLiteral literal) {} @override - void visitDoubleLiteral(ir.DoubleLiteral literal) => add(literal); + void visitDoubleLiteral(ir.DoubleLiteral literal) {} @override - void visitBoolLiteral(ir.BoolLiteral literal) => add(literal); + void visitBoolLiteral(ir.BoolLiteral literal) {} @override - void visitStringLiteral(ir.StringLiteral literal) => add(literal); + void visitStringLiteral(ir.StringLiteral literal) {} @override void visitSymbolLiteral(ir.SymbolLiteral literal) => add(literal); diff --git a/pkg/compiler/lib/src/kernel/kernel_strategy.dart b/pkg/compiler/lib/src/kernel/kernel_strategy.dart index 2146eba9424..f58417ecffe 100644 --- a/pkg/compiler/lib/src/kernel/kernel_strategy.dart +++ b/pkg/compiler/lib/src/kernel/kernel_strategy.dart @@ -248,7 +248,7 @@ class KernelWorkItem implements ResolutionWorkItem { WorldImpact worldImpact = _impactTransformer.transformResolutionImpact(impact); if (impactCache != null) { - impactCache[element] = impact; + impactCache[element] = worldImpact; } return worldImpact; }); diff --git a/pkg/compiler/lib/src/resolution/deferred_load.dart b/pkg/compiler/lib/src/resolution/deferred_load.dart index 04e4fbfae02..5824e169ae1 100644 --- a/pkg/compiler/lib/src/resolution/deferred_load.dart +++ b/pkg/compiler/lib/src/resolution/deferred_load.dart @@ -15,6 +15,7 @@ import '../elements/elements.dart' AstElement, AccessorElement, ClassElement, + ConstructorElement, Element, ExportElement, ImportElement, @@ -145,6 +146,11 @@ class AstDeferredLoadTask extends DeferredLoadTask { TreeElements treeElements = element.resolvedAst.elements; assert(treeElements != null); + Set metadataNodes = element.implementation.metadata + .map((m) => m.node) + .toSet() + ..addAll(element.declaration.metadata.map((m) => m.node)); + // TODO(johnniwinther): Add only expressions that are actually needed. // Currently we have some noise here: Some potential expressions are // seen that should never be added (for instance field initializers @@ -154,6 +160,7 @@ class AstDeferredLoadTask extends DeferredLoadTask { // See dartbug.com/26406 for context. treeElements .forEachConstantNode((ast.Node node, ConstantExpression expression) { + if (metadataNodes.contains(node)) return; if (compiler.serialization.isDeserialized(element)) { if (!expression.isPotential) { // Enforce evaluation of [expression]. @@ -366,4 +373,10 @@ class AstDeferredLoadTask extends DeferredLoadTask { assert(result != null); return result; } + + bool ignoreEntityInDump(covariant Element element) => + // origin element will already be dumped. + element.isPatch || + // redirecting factories are not visible to the kernel ir + element is ConstructorElement && element.isRedirectingFactory; } diff --git a/pkg/compiler/lib/src/ssa/builder_kernel.dart b/pkg/compiler/lib/src/ssa/builder_kernel.dart index dc05ad5ea62..9807d3b099d 100644 --- a/pkg/compiler/lib/src/ssa/builder_kernel.dart +++ b/pkg/compiler/lib/src/ssa/builder_kernel.dart @@ -2612,7 +2612,8 @@ class KernelSsaGraphBuilder extends ir.Visitor // there is no prefix the old FE wouldn't treat this in any special // way. Also, if the prefix points to a constant in the main output // unit, the old FE would still generate a deferred wrapper here. - if (!unit.isMainOutput) { + if (!compiler.backend.outputUnitData + .hasOnlyNonDeferredImportPaths(targetElement, field)) { stack.add(graph.addDeferredConstant( value, unit, sourceInformation, compiler, closedWorld)); } else { diff --git a/tests/compiler/dart2js/deferred_loading/deferred_loading_test.dart b/tests/compiler/dart2js/deferred_loading/deferred_loading_test.dart index 59b43ee8d1f..c4f9e1a89d1 100644 --- a/tests/compiler/dart2js/deferred_loading/deferred_loading_test.dart +++ b/tests/compiler/dart2js/deferred_loading/deferred_loading_test.dart @@ -18,9 +18,7 @@ import 'package:compiler/src/constants/values.dart'; import 'package:kernel/ast.dart' as ir; -const List skipForKernel = const [ - 'dont_inline_deferred_constants.dart', -]; +const List skipForKernel = const []; /// Add in options to pass to the compiler like /// `Flags.disableTypeInference` or `Flags.disableInlining` @@ -97,6 +95,7 @@ void computeAstOutputUnitData( if (member is FieldElement && member.isConst) { var node = member.initializer; var constant = compiler.constants.getConstantValue(member.constant); + if (constant.isPrimitive) return; _registerValue( new NodeId(node.getBeginToken().charOffset, IdKind.node), outputUnitString(data.outputUnitForConstant(constant)), @@ -136,6 +135,7 @@ void computeKernelOutputUnitData( if (memberNode is ir.Field && memberNode.isConst) { ir.Expression node = memberNode.initializer; ConstantValue constant = elementMap.getConstantValue(node); + if (constant.isPrimitive) return; SourceSpan span = computeSourceSpanFromTreeNode(node); if (node is ir.ConstructorInvocation || node is ir.ListLiteral || diff --git a/tests/compiler/dart2js/deferred_loading/libs/deferred_constant1_lib3.dart b/tests/compiler/dart2js/deferred_loading/libs/deferred_constant1_lib3.dart index a3a279dc2fb..1cb567c986e 100644 --- a/tests/compiler/dart2js/deferred_loading/libs/deferred_constant1_lib3.dart +++ b/tests/compiler/dart2js/deferred_loading/libs/deferred_constant1_lib3.dart @@ -48,7 +48,7 @@ const C5 = /*OutputUnit(main, {})*/ const C(5); /// --------------------------------------------------------------------------- /*element: C6:OutputUnit(1, {lib2})*/ -const C6 = /*OutputUnit(1, {lib2})*/ "string6"; +const C6 = "string6"; /*element: C7:OutputUnit(1, {lib2})*/ const C7 = /*OutputUnit(1, {lib2})*/ const C(const C(7)); diff --git a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib1.dart b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib1.dart index 5f3267e6e2d..1fefeae7854 100644 --- a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib1.dart +++ b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib1.dart @@ -6,18 +6,27 @@ import "dont_inline_deferred_constants_main.dart" show C; import "dont_inline_deferred_constants_main.dart" as main; /*element: C1:OutputUnit(1, {lib1})*/ -const C1 = /*OutputUnit(1, {lib1})*/ "string1"; +const C1 = "string1"; + +/*element: C1b:OutputUnit(1, {lib1})*/ +const C1b = /*OutputUnit(1, {lib1})*/ const C("string1"); /*element: C2:OutputUnit(1, {lib1})*/ -const C2 = /*OutputUnit(1, {lib1})*/ 1010; +const C2 = 1010; + +/*element: C2b:OutputUnit(1, {lib1})*/ +const C2b = /*OutputUnit(1, {lib1})*/ const C(1010); class D { /*element: D.C3:OutputUnit(1, {lib1})*/ - static const C3 = /*OutputUnit(1, {lib1})*/ "string2"; + static const C3 = "string2"; + + /*element: D.C3b:OutputUnit(1, {lib1})*/ + static const C3b = /*OutputUnit(1, {lib1})*/ const C("string2"); } /*element: C4:OutputUnit(1, {lib1})*/ -const C4 = /*OutputUnit(main, {})*/ "string4"; +const C4 = "string4"; /*element: C5:OutputUnit(1, {lib1})*/ const C5 = /*OutputUnit(main, {})*/ const C(1); diff --git a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib2.dart b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib2.dart index c53f32a809b..c1d65c58dd3 100644 --- a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib2.dart +++ b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_lib2.dart @@ -6,7 +6,7 @@ import "dont_inline_deferred_constants_main.dart" show C; import "dont_inline_deferred_constants_main.dart" as main; /*element: C4:OutputUnit(3, {lib2})*/ -const C4 = /*OutputUnit(main, {})*/ "string4"; +const C4 = "string4"; /*element: C5:OutputUnit(3, {lib2})*/ const C5 = /*OutputUnit(main, {})*/ const C(1); diff --git a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_main.dart b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_main.dart index 4e781f53dec..a68724afd7c 100644 --- a/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_main.dart +++ b/tests/compiler/dart2js/deferred_loading/libs/dont_inline_deferred_constants_main.dart @@ -6,7 +6,7 @@ import 'dont_inline_deferred_constants_lib1.dart' deferred as lib1; import 'dont_inline_deferred_constants_lib2.dart' deferred as lib2; /*element: c:OutputUnit(main, {})*/ -const c = /*OutputUnit(main, {})*/ "string3"; +const c = "string3"; class C { /*element: C.p:OutputUnit(main, {})*/ @@ -26,8 +26,11 @@ void main() { lib1.foo(); lib2.foo(); print(lib1.C1); + print(lib1.C1b); print(lib1.C2); + print(lib1.C2b); print(lib1.D.C3); + print(lib1.D.C3b); print(c); print(lib1.C4); print(lib2.C4);