This is an automated email from the ASF dual-hosted git repository. ddekany pushed a commit to branch 2.3-gae in repository https://gitbox.apache.org/repos/asf/freemarker.git
commit e7d91e63596c97a59568af78467f1854b3b11a2f Author: ddekany <[email protected]> AuthorDate: Sun Mar 31 18:17:04 2019 +0200 Made ?size smarter when its result is compared to an integer literal. Specifically, for ?filter-ed sequences (or for the result of similar future Stream-API-like built-ins) it won't count more elements then it's necessary to tell the comparison result. Also, for a TemplateCollectionModelEx, it calls isEmpty() instead of size(), if for the comparison result we only have to know if we have more than 0 elements. --- src/main/java/freemarker/core/BuiltIn.java | 5 +- .../freemarker/core/BuiltInsForMultipleTypes.java | 37 +++++++++++- .../java/freemarker/core/ComparisonExpression.java | 16 +++++ src/main/java/freemarker/core/EvalUtil.java | 12 ++++ src/main/java/freemarker/core/IteratorBlock.java | 6 +- src/main/java/freemarker/core/MiscUtil.java | 7 +++ src/manual/en_US/book.xml | 15 +++++ ...ilyGeneratedSeqTargetSupportInBuiltinsTest.java | 69 +++++++++++++++++++++- 8 files changed, 158 insertions(+), 9 deletions(-) diff --git a/src/main/java/freemarker/core/BuiltIn.java b/src/main/java/freemarker/core/BuiltIn.java index 0679e21..9e3dd3c 100644 --- a/src/main/java/freemarker/core/BuiltIn.java +++ b/src/main/java/freemarker/core/BuiltIn.java @@ -388,8 +388,9 @@ abstract class BuiltIn extends Expression implements Cloneable { bi.key = key; bi.target = target; if (bi.isLazilyGeneratedSequenceModelTargetSupported()) { - if (target instanceof BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) { - ((BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) target) + Expression cleanedTarget = MiscUtil.peelParentheses(target); + if (cleanedTarget instanceof BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) { + ((BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) cleanedTarget) .setLazyResultGenerationAllowed(true); } } diff --git a/src/main/java/freemarker/core/BuiltInsForMultipleTypes.java b/src/main/java/freemarker/core/BuiltInsForMultipleTypes.java index 61f8f3f..b690cf5 100644 --- a/src/main/java/freemarker/core/BuiltInsForMultipleTypes.java +++ b/src/main/java/freemarker/core/BuiltInsForMultipleTypes.java @@ -48,6 +48,7 @@ import freemarker.template.TemplateScalarModel; import freemarker.template.TemplateSequenceModel; import freemarker.template.TemplateTransformModel; import freemarker.template._TemplateAPI; +import freemarker.template.utility.NumberUtil; /** * A holder for builtins that didn't fit into any other category. @@ -488,12 +489,16 @@ class BuiltInsForMultipleTypes { return true; } + private int countingLimit; + @Override TemplateModel _eval(Environment env) throws TemplateException { TemplateModel model = target.eval(env); final int size; - if (model instanceof TemplateSequenceModel) { + if (countingLimit == 1 && model instanceof TemplateCollectionModelEx) { + size = ((TemplateCollectionModelEx) model).isEmpty() ? 0 : 1; + } else if (model instanceof TemplateSequenceModel) { size = ((TemplateSequenceModel) model).size(); } else if (model instanceof TemplateCollectionModelEx) { size = ((TemplateCollectionModelEx) model).size(); @@ -508,8 +513,11 @@ class BuiltInsForMultipleTypes { // expected by the template author, and we just made that calculation less wasteful here. TemplateModelIterator iterator = ((LazilyGeneratedSequenceModel) model).iterator(); int counter = 0; - while (iterator.hasNext()) { + countElements: while (iterator.hasNext()) { counter++; + if (counter == countingLimit) { + break countElements; + } iterator.next(); } size = counter; @@ -526,6 +534,31 @@ class BuiltInsForMultipleTypes { } return new SimpleNumber(size); } + + /** + * Enables an optimization trick when the result of the built-in will be compared with a number literal. + * For example, in the case of {@code things?size != 0} we only need to check of the target is non-empty, which + * is often more efficient than telling exactly how many elements it has. + */ + void setCountingLimit(int cmpOperator, NumberLiteral rightOperand) { + int cmpInt; + try { + cmpInt = NumberUtil.toIntExact(rightOperand.getAsNumber()); + } catch (ArithmeticException e) { + // As we can't know what the ArithmeticEngine will be on runtime, we won't risk this for non-integers. + return; + } + + switch (cmpOperator) { + case EvalUtil.CMP_OP_EQUALS: countingLimit = cmpInt + 1; break; + case EvalUtil.CMP_OP_NOT_EQUALS: countingLimit = cmpInt + 1; break; + case EvalUtil.CMP_OP_LESS_THAN: countingLimit = cmpInt; break; + case EvalUtil.CMP_OP_GREATER_THAN: countingLimit = cmpInt + 1; break; + case EvalUtil.CMP_OP_LESS_THAN_EQUALS: countingLimit = cmpInt + 1; break; + case EvalUtil.CMP_OP_GREATER_THAN_EQUALS: countingLimit = cmpInt; break; + default: throw new BugException("Unsupported comparator operator code: " + cmpOperator); + } + } } static class stringBI extends BuiltIn { diff --git a/src/main/java/freemarker/core/ComparisonExpression.java b/src/main/java/freemarker/core/ComparisonExpression.java index f647ce4..b33c5bc 100644 --- a/src/main/java/freemarker/core/ComparisonExpression.java +++ b/src/main/java/freemarker/core/ComparisonExpression.java @@ -51,6 +51,22 @@ final class ComparisonExpression extends BooleanExpression { } else { throw new BugException("Unknown comparison operator " + opString); } + + Expression cleanedLeft = MiscUtil.peelParentheses(left); + Expression cleanedRight = MiscUtil.peelParentheses(right); + if (cleanedLeft instanceof BuiltInsForMultipleTypes.sizeBI) { + if (cleanedRight instanceof NumberLiteral) { + ((BuiltInsForMultipleTypes.sizeBI) cleanedLeft).setCountingLimit( + operation, (NumberLiteral) cleanedRight + ); + } + } else if (cleanedRight instanceof BuiltInsForMultipleTypes.sizeBI) { + if (cleanedLeft instanceof NumberLiteral) { + ((BuiltInsForMultipleTypes.sizeBI) cleanedRight).setCountingLimit( + EvalUtil.mirrorCmpOperator(operation), (NumberLiteral) cleanedLeft + ); + } + } } /* diff --git a/src/main/java/freemarker/core/EvalUtil.java b/src/main/java/freemarker/core/EvalUtil.java index ebde9c3..0fa7a48 100644 --- a/src/main/java/freemarker/core/EvalUtil.java +++ b/src/main/java/freemarker/core/EvalUtil.java @@ -343,6 +343,18 @@ class EvalUtil { } } + static int mirrorCmpOperator(int operator) { + switch (operator) { + case CMP_OP_EQUALS: return CMP_OP_EQUALS; + case CMP_OP_NOT_EQUALS: return CMP_OP_NOT_EQUALS; + case CMP_OP_LESS_THAN: return CMP_OP_GREATER_THAN; + case CMP_OP_GREATER_THAN: return CMP_OP_LESS_THAN; + case CMP_OP_LESS_THAN_EQUALS: return CMP_OP_GREATER_THAN_EQUALS; + case CMP_OP_GREATER_THAN_EQUALS: return CMP_OP_LESS_THAN_EQUALS; + default: throw new BugException("Unsupported comparator operator code: " + operator); + } + } + /** * Converts a value to plain text {@link String}, or a {@link TemplateMarkupOutputModel} if that's what the * {@link TemplateValueFormat} involved produces. diff --git a/src/main/java/freemarker/core/IteratorBlock.java b/src/main/java/freemarker/core/IteratorBlock.java index a51401a..c734cbe 100644 --- a/src/main/java/freemarker/core/IteratorBlock.java +++ b/src/main/java/freemarker/core/IteratorBlock.java @@ -83,8 +83,10 @@ final class IteratorBlock extends TemplateElement { this.hashListing = hashListing; this.forEach = forEach; - if (listedExp instanceof BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) { - ((BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) listedExp).setLazyResultGenerationAllowed(true); + Expression cleanedListExp = MiscUtil.peelParentheses(listedExp); + if (cleanedListExp instanceof BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) { + ((BuiltInsForSequences.IntermediateStreamOperationLikeBuiltIn) cleanedListExp) + .setLazyResultGenerationAllowed(true); fetchElementsOutsideLoopVarContext = true; } else { fetchElementsOutsideLoopVarContext = false; diff --git a/src/main/java/freemarker/core/MiscUtil.java b/src/main/java/freemarker/core/MiscUtil.java index 770e394..505edb4 100644 --- a/src/main/java/freemarker/core/MiscUtil.java +++ b/src/main/java/freemarker/core/MiscUtil.java @@ -64,5 +64,12 @@ class MiscUtil { }); return res; } + + static Expression peelParentheses(Expression exp) { + while (exp instanceof ParentheticalExpression) { + exp = ((ParentheticalExpression) exp).getNestedExpression(); + } + return exp; + } } diff --git a/src/manual/en_US/book.xml b/src/manual/en_US/book.xml index 0633247..24ceada 100644 --- a/src/manual/en_US/book.xml +++ b/src/manual/en_US/book.xml @@ -27963,6 +27963,21 @@ TemplateModel x = env.getVariable("x"); // get variable x</programlisting> </listitem> <listitem> + <para>If the result of + <literal><replaceable>seq</replaceable>?size</literal> is + compared to an integer literal in a template, like in + <literal><replaceable>seq</replaceable>?size != 0</literal>, or + <literal><replaceable>seq</replaceable>?size < 1</literal>, + and to decide the answer it's enough to know if + <literal><replaceable>seq</replaceable></literal> is an empty + sequence or collection (i.e., the exact size isn't needed), and + said value implements + <literal>TemplateCollectionModelEx</literal>, FreeMarker will + call <literal>TemplateCollectionModelEx.isEmpty()</literal> + instead of <literal>size()</literal>.</para> + </listitem> + + <listitem> <para>Added <literal>TemplateModelUtils.wrapAsHashUnion(ObjectWrapper, List<?>)</literal> and diff --git a/src/test/java/freemarker/core/LazilyGeneratedSeqTargetSupportInBuiltinsTest.java b/src/test/java/freemarker/core/LazilyGeneratedSeqTargetSupportInBuiltinsTest.java index 37cefa8..36af95f 100644 --- a/src/test/java/freemarker/core/LazilyGeneratedSeqTargetSupportInBuiltinsTest.java +++ b/src/test/java/freemarker/core/LazilyGeneratedSeqTargetSupportInBuiltinsTest.java @@ -43,7 +43,7 @@ import freemarker.test.TemplateTest; public class LazilyGeneratedSeqTargetSupportInBuiltinsTest extends TemplateTest { @Test - public void sizeTest() throws Exception { + public void sizeBasicsTest() throws Exception { assertOutput("${seq?size}", "[size]3"); assertOutput("${collEx?size}", @@ -63,6 +63,56 @@ public class LazilyGeneratedSeqTargetSupportInBuiltinsTest extends TemplateTest } @Test + public void sizeComparisonTest() throws Exception { + // These actually aren't related to lazy generation... + assertOutput("${collEx?size}", + "[size]3"); + assertOutput("${collEx?size != 0}", + "[isEmpty]true"); + assertOutput("${0 != collEx?size}", + "[isEmpty]true"); + assertOutput("${collEx?size == 0}", + "[isEmpty]false"); + assertOutput("${0 == collEx?size}", + "[isEmpty]false"); + assertOutput("${(collEx?size >= 1)}", + "[isEmpty]true"); + assertOutput("${1 <= collEx?size}", + "[isEmpty]true"); + assertOutput("${collEx?size <= 0}", + "[isEmpty]false"); + assertOutput("${(0 >= collEx?size)}", + "[isEmpty]false"); + assertOutput("${collEx?size > 0}", + "[isEmpty]true"); + assertOutput("${0 < collEx?size}", + "[isEmpty]true"); + assertOutput("${collEx?size < 1}", + "[isEmpty]false"); + assertOutput("${1 > collEx?size}", + "[isEmpty]false"); + assertOutput("${collEx?size == 1}", + "[size]false"); + assertOutput("${1 == collEx?size}", + "[size]false"); + + // Now the lazy generation things: + assertOutput("${collLong?filter(x -> true)?size}", + "[iterator]" + + "[hasNext][next][hasNext][next][hasNext][next]" + + "[hasNext][next][hasNext][next][hasNext][next][hasNext]6"); + // Note: "[next]" is added by ?filter, as it has to know if the element matches the predicate. + assertOutput("${collLong?filter(x -> true)?size != 0}", + "[iterator][hasNext][next]true"); + assertOutput("${collLong?filter(x -> true)?size != 1}", + "[iterator][hasNext][next][hasNext][next]true"); + assertOutput("${collLong?filter(x -> true)?size == 1}", + "[iterator][hasNext][next][hasNext][next]false"); + assertOutput("${collLong?filter(x -> true)?size < 3}", + "[iterator][hasNext][next][hasNext][next][hasNext][next]false"); + } + + @Test public void firstTest() throws Exception { assertOutput("${coll?first}", "[iterator][hasNext][next]1"); @@ -87,13 +137,26 @@ public class LazilyGeneratedSeqTargetSupportInBuiltinsTest extends TemplateTest @Test public void listTest() throws Exception { // Note: #list has to fetch elements up to 4 to know if it?hasNext - assertOutput("<#list collLong?filter(x -> x % 2 == 0) as it>${it} ${it?hasNext?c}<#break></#list>", - "[iterator][hasNext][next][hasNext][next][hasNext][next][hasNext][next]2 true"); + String commonSourceExp = "collLong?filter(x -> x % 2 == 0)"; + for (String sourceExp : new String[] {commonSourceExp, "(" + commonSourceExp + ")"}) { + assertOutput("<#list " + sourceExp + " as it>${it} ${it?hasNext}<#break></#list>", + "[iterator][hasNext][next][hasNext][next][hasNext][next][hasNext][next]2 true"); + } } + @Test + public void biTargetParenthesisTest() throws Exception { + assertOutput("${(coll?filter(x -> x % 2 == 0))?first}", + "[iterator][hasNext][next][hasNext][next]2"); + } + + + @Override protected Configuration createConfiguration() throws Exception { Configuration cfg = super.createConfiguration(); + cfg.setIncompatibleImprovements(Configuration.VERSION_2_3_29); + cfg.setBooleanFormat("c"); cfg.setSharedVariable("seq", new MonitoredTemplateSequenceModel(1, 2, 3)); cfg.setSharedVariable("coll", new MonitoredTemplateCollectionModel(1, 2, 3)); cfg.setSharedVariable("collLong", new MonitoredTemplateCollectionModel(1, 2, 3, 4, 5, 6));
