From 97b2878b7368224b921c8a8df1aba38b2a35a445 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 25 Feb 2021 14:45:59 -0800 Subject: [PATCH 1/2] Mustachio: avoid calling Iterable getter multiple times Take this template, where `widgets` is an Iterable property: Text {{#widgets}} {{name}} {{/widgets}}. A mustache renderer should assume that the `widgets` getter may have side effects, and it should only be called one time. Currently, Mustachio will call this getter 2 or 3 times, as the behavior differs if the section is inverted, and if the Iterable is empty. This change is actually quite readable, and simplifies the code. The magic sauce is in the lazy Iterable returned by `renderIterable`. If Mustachio does not wish to render the elements, then the lazy map will not be evaluated. --- lib/src/generator/templates.renderers.dart | 171 +++++-------------- lib/src/mustachio/renderer_base.dart | 22 ++- test/mustachio/builder_test.dart | 7 +- test/mustachio/foo.renderers.dart | 7 +- test/mustachio/renderer_test.dart | 21 --- tool/mustachio/codegen_runtime_renderer.dart | 8 +- 6 files changed, 58 insertions(+), 178 deletions(-) diff --git a/lib/src/generator/templates.renderers.dart b/lib/src/generator/templates.renderers.dart index 43bb86c607..77f53085a5 100644 --- a/lib/src/generator/templates.renderers.dart +++ b/lib/src/generator/templates.renderers.dart @@ -89,14 +89,10 @@ class _Renderer_PackageTemplateData extends RendererBase { (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.navLinks?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.navLinks) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.navLinks + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'package': Property( @@ -175,14 +171,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Set'), - isEmptyIterable: (CT_ c) => c.allLibraries?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.allLibraries) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.allLibraries + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'baseHref': Property( @@ -210,14 +202,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.categories?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.categories) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.categories + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'categoriesWithPublicLibraries': Property( @@ -226,16 +214,10 @@ class _Renderer_Package extends RendererBase { (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => - c.categoriesWithPublicLibraries?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.categoriesWithPublicLibraries) { - buffer.write( - _render_LibraryContainer(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.categoriesWithPublicLibraries.map( + (e) => _render_LibraryContainer(e, ast, r.template, parent: r)); }, ), 'config': Property( @@ -254,14 +236,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.containerOrder?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.containerOrder) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.containerOrder + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'defaultCategory': Property( @@ -318,14 +296,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.documentationFrom?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.documentationFrom) { - buffer.write(_render_Locatable(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.documentationFrom + .map((e) => _render_Locatable(e, ast, r.template, parent: r)); }, ), 'documentedCategories': Property( @@ -334,14 +308,10 @@ class _Renderer_Package extends RendererBase { (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => c.documentedCategories?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.documentedCategories) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.documentedCategories + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'documentedCategoriesSorted': Property( @@ -350,15 +320,10 @@ class _Renderer_Package extends RendererBase { (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => - c.documentedCategoriesSorted?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.documentedCategoriesSorted) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.documentedCategoriesSorted + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'documentedWhere': Property( @@ -567,14 +532,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Set'), - isEmptyIterable: (CT_ c) => c.locationPieces?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.locationPieces) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.locationPieces + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'name': Property( @@ -658,14 +619,10 @@ class _Renderer_Package extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => c.publicLibraries?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.publicLibraries) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.publicLibraries + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'toolInvocationIndex': Property( @@ -733,14 +690,10 @@ class _Renderer_Locatable extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.documentationFrom?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.documentationFrom) { - buffer.write(_render_Locatable(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.documentationFrom + .map((e) => _render_Locatable(e, ast, r.template, parent: r)); }, ), 'documentationIsLocal': Property( @@ -815,14 +768,10 @@ class _Renderer_LibraryContainer extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.containerOrder?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.containerOrder) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.containerOrder + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'enclosingName': Property( @@ -854,14 +803,10 @@ class _Renderer_LibraryContainer extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.libraries?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.libraries) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.libraries + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'packageGraph': Property( @@ -879,14 +824,10 @@ class _Renderer_LibraryContainer extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => c.publicLibraries?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.publicLibraries) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.publicLibraries + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'publicLibrariesSorted': Property( @@ -894,14 +835,10 @@ class _Renderer_LibraryContainer extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Iterable'), - isEmptyIterable: (CT_ c) => c.publicLibrariesSorted?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.publicLibrariesSorted) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.publicLibrariesSorted + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'sortKey': Property( @@ -1044,14 +981,10 @@ class _Renderer_Nameable extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Set'), - isEmptyIterable: (CT_ c) => c.namePieces?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.namePieces) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.namePieces + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), }; @@ -1099,14 +1032,10 @@ class _Renderer_Canonicalization extends RendererBase { (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.commentRefs?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.commentRefs) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.commentRefs + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'isCanonical': Property( @@ -1121,14 +1050,10 @@ class _Renderer_Canonicalization extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'Set'), - isEmptyIterable: (CT_ c) => c.locationPieces?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.locationPieces) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.locationPieces + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), }; @@ -1299,14 +1224,10 @@ class _Renderer_TemplateData renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.localPackages?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.localPackages) { - buffer.write(_render_Package(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.localPackages + .map((e) => _render_Package(e, ast, r.template, parent: r)); }, ), 'metaDescription': Property( @@ -1325,14 +1246,10 @@ class _Renderer_TemplateData (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable( c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.navLinks?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.navLinks) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.navLinks + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'navLinksWithGenerics': Property( @@ -1340,14 +1257,10 @@ class _Renderer_TemplateData renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.navLinksWithGenerics?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.navLinksWithGenerics) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.navLinksWithGenerics + .map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'parent': Property( diff --git a/lib/src/mustachio/renderer_base.dart b/lib/src/mustachio/renderer_base.dart index 9a99d940cd..9e8fda2ee8 100644 --- a/lib/src/mustachio/renderer_base.dart +++ b/lib/src/mustachio/renderer_base.dart @@ -230,13 +230,20 @@ abstract class RendererBase { } if (property.renderIterable != null) { - // An inverted section is rendered with the current context. - if (node.invert && property.isEmptyIterable(context)) { + var renderedIterable = + property.renderIterable(context, this, node.children); + if (node.invert && renderedIterable.isEmpty) { + // An inverted section is rendered with the current context. renderBlock(node.children); + } else if (!node.invert && renderedIterable.isNotEmpty) { + var buffer = StringBuffer(); + for (var renderedElement in renderedIterable) { + buffer.write(renderedElement); + } + write(buffer.toString()); } - if (!node.invert && !property.isEmptyIterable(context)) { - write(property.renderIterable(context, this, node.children)); - } + // Otherwise, render nothing. + return; } @@ -302,9 +309,7 @@ class Property { /// object [context]. final bool /*!*/ Function(T context) /*?*/ getBool; - final bool /*!*/ Function(T) /*?*/ isEmptyIterable; - - final String /*!*/ Function( + final Iterable /*!*/ Function( T, RendererBase, List /*!*/) /*?*/ renderIterable; @@ -317,7 +322,6 @@ class Property { {@required this.getValue, this.renderVariable, this.getBool, - this.isEmptyIterable, this.renderIterable, this.isNullValue, this.renderValue}); diff --git a/test/mustachio/builder_test.dart b/test/mustachio/builder_test.dart index 98c10d7fbc..02323738f4 100644 --- a/test/mustachio/builder_test.dart +++ b/test/mustachio/builder_test.dart @@ -176,14 +176,9 @@ class Baz {} renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.l1?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.l1) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.l1.map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), ''')); diff --git a/test/mustachio/foo.renderers.dart b/test/mustachio/foo.renderers.dart index 9c1f1b5f38..108d937ff4 100644 --- a/test/mustachio/foo.renderers.dart +++ b/test/mustachio/foo.renderers.dart @@ -53,14 +53,9 @@ class Renderer_Foo extends RendererBase { renderVariable: (CT_ c, Property self, List remainingNames) => self.renderSimpleVariable(c, remainingNames, 'List'), - isEmptyIterable: (CT_ c) => c.l1?.isEmpty ?? true, renderIterable: (CT_ c, RendererBase r, List ast) { - var buffer = StringBuffer(); - for (var e in c.l1) { - buffer.write(renderSimple(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.l1.map((e) => renderSimple(e, ast, r.template, parent: r)); }, ), 'p1': Property( diff --git a/test/mustachio/renderer_test.dart b/test/mustachio/renderer_test.dart index 92a28968e2..0991518368 100644 --- a/test/mustachio/renderer_test.dart +++ b/test/mustachio/renderer_test.dart @@ -36,7 +36,6 @@ void main() { expect(propertyMap['b1'].getValue, isNotNull); expect(propertyMap['b1'].renderVariable, isNotNull); expect(propertyMap['b1'].getBool, isNotNull); - expect(propertyMap['b1'].isEmptyIterable, isNull); expect(propertyMap['b1'].renderIterable, isNull); expect(propertyMap['b1'].isNullValue, isNull); expect(propertyMap['b1'].renderValue, isNull); @@ -47,7 +46,6 @@ void main() { expect(propertyMap['l1'].getValue, isNotNull); expect(propertyMap['l1'].renderVariable, isNotNull); expect(propertyMap['l1'].getBool, isNull); - expect(propertyMap['l1'].isEmptyIterable, isNotNull); expect(propertyMap['l1'].renderIterable, isNotNull); expect(propertyMap['l1'].isNullValue, isNull); expect(propertyMap['l1'].renderValue, isNull); @@ -58,7 +56,6 @@ void main() { expect(propertyMap['s1'].getValue, isNotNull); expect(propertyMap['s1'].renderVariable, isNotNull); expect(propertyMap['s1'].getBool, isNull); - expect(propertyMap['s1'].isEmptyIterable, isNull); expect(propertyMap['s1'].renderIterable, isNull); expect(propertyMap['s1'].isNullValue, isNotNull); expect(propertyMap['s1'].renderValue, isNotNull); @@ -76,24 +73,6 @@ void main() { expect(propertyMap['b1'].getBool(foo), isTrue); }); - test('isEmptyIterable returns true when an Iterable value is empty', () { - var propertyMap = Renderer_Foo.propertyMap(); - var foo = Foo()..l1 = []; - expect(propertyMap['l1'].isEmptyIterable(foo), isTrue); - }); - - test('isEmptyIterable returns false when an Iterable value is not empty', () { - var propertyMap = Renderer_Foo.propertyMap(); - var foo = Foo()..l1 = [1, 2, 3]; - expect(propertyMap['l1'].isEmptyIterable(foo), isFalse); - }); - - test('isEmptyIterable returns true when an Iterable value is null', () { - var propertyMap = Renderer_Foo.propertyMap(); - var foo = Foo()..l1 = null; - expect(propertyMap['l1'].isEmptyIterable(foo), isTrue); - }); - test('isNullValue returns true when a value is null', () { var propertyMap = Renderer_Foo.propertyMap(); var foo = Foo()..s1 = null; diff --git a/tool/mustachio/codegen_runtime_renderer.dart b/tool/mustachio/codegen_runtime_renderer.dart index fe2feeab4a..7f5ef19b2b 100644 --- a/tool/mustachio/codegen_runtime_renderer.dart +++ b/tool/mustachio/codegen_runtime_renderer.dart @@ -456,15 +456,9 @@ renderVariable: var rendererName = _typeToRenderFunctionName[innerType.element] ?? 'renderSimple'; _buffer.writeln(''' -isEmptyIterable: ($_contextTypeVariable c) => c.$getterName?.isEmpty ?? true, - renderIterable: ($_contextTypeVariable c, RendererBase<$_contextTypeVariable> r, List ast) { - var buffer = StringBuffer(); - for (var e in c.$getterName) { - buffer.write($rendererName(e, ast, r.template, parent: r)); - } - return buffer.toString(); + return c.$getterName.map((e) => $rendererName(e, ast, r.template, parent: r)); }, '''); } From a4cc63ee4f6e74adec6a27e477b2d28a3259dfec Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Fri, 26 Feb 2021 08:03:23 -0800 Subject: [PATCH 2/2] feedback --- lib/src/mustachio/renderer_base.dart | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/src/mustachio/renderer_base.dart b/lib/src/mustachio/renderer_base.dart index 9e8fda2ee8..e3096db45d 100644 --- a/lib/src/mustachio/renderer_base.dart +++ b/lib/src/mustachio/renderer_base.dart @@ -236,10 +236,7 @@ abstract class RendererBase { // An inverted section is rendered with the current context. renderBlock(node.children); } else if (!node.invert && renderedIterable.isNotEmpty) { - var buffer = StringBuffer(); - for (var renderedElement in renderedIterable) { - buffer.write(renderedElement); - } + var buffer = StringBuffer()..writeAll(renderedIterable); write(buffer.toString()); } // Otherwise, render nothing.