Repository: PebbleTemplates/pebble
Pull request #348 (2018-06-07 13:22:28)

Optimize ScopeChain for basic case with single scope
Motivation:

`ScopeChain#get` performs `containsKey` then `get` so it can account for null values.

The current implementation tries to optimize for the single scope case for the chain scanning with saving an Iterator.
It's possible to go further this way.

Modification:

Optimize for the most common use case where the key is defined and there's a single scope.
Only perform a `get` and only check `contains` if the value is null and there is more than one scope.

Result:

Better performance.


Patch:
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/ArrayResolver.java b/src/main/java/com/mitchellbosecke/pebble/attributes/ArrayResolver.java
index 49cb10a6..e7f3ae34 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/ArrayResolver.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/ArrayResolver.java
@@ -29,10 +29,10 @@ public class ArrayResolver implements AttributeResolver {
                 "Index out of bounds while accessing array with strict variables on.",
                 attributeName, lineNumber, filename);
       } else {
-        return () -> null;
+        return new ResolvedAttribute(null);
       }
     }
-    return () -> Array.get(instance, index);
+    return new ResolvedAttribute(Array.get(instance, index));
   }
 
   private int getIndex(String attributeName) {
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java b/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
index 88f48180..5d00b6a7 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/DefaultAttributeResolver.java
@@ -54,8 +54,7 @@ public class DefaultAttributeResolver implements AttributeResolver {
       }
 
       if (member != null) {
-        Member finalMember = member;
-        return () -> this.invokeMember(instance, finalMember, argumentValues);
+        return new ResolvedAttribute(invokeMember(instance, member, argumentValues));
       }
     }
     return null;
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/ListResolver.java b/src/main/java/com/mitchellbosecke/pebble/attributes/ListResolver.java
index e5b4a041..e02dbc12 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/ListResolver.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/ListResolver.java
@@ -33,11 +33,11 @@ public class ListResolver implements AttributeResolver {
                 "Index out of bounds while accessing array with strict variables on.",
                 attributeName, lineNumber, filename);
       } else {
-        return () -> null;
+        return new ResolvedAttribute(null);
       }
     }
 
-    return () -> list.get(index);
+    return new ResolvedAttribute(list.get(index));
   }
 
   private int getIndex(String attributeName) {
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/MacroResolver.java b/src/main/java/com/mitchellbosecke/pebble/attributes/MacroResolver.java
index 43f01d46..7600dcae 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/MacroResolver.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/MacroResolver.java
@@ -20,6 +20,6 @@ public class MacroResolver implements AttributeResolver {
                                    int lineNumber) {
     MacroAttributeProvider macroAttributeProvider = (MacroAttributeProvider) instance;
     String attributeName = String.valueOf(attributeNameValue);
-    return () -> macroAttributeProvider.macro(context, attributeName, args, false, lineNumber);
+    return new ResolvedAttribute(macroAttributeProvider.macro(context, attributeName, args, false, lineNumber));
   }
 }
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/MapResolver.java b/src/main/java/com/mitchellbosecke/pebble/attributes/MapResolver.java
index 6d1321ab..7c2ffcb3 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/MapResolver.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/MapResolver.java
@@ -29,9 +29,9 @@ public class MapResolver implements AttributeResolver {
 
       Class<?> keyClass = object.keySet().iterator().next().getClass();
       Object key = this.cast(keyAsNumber, keyClass, filename, lineNumber);
-      return () -> object.get(key);
+      return new ResolvedAttribute(object.get(key));
     }
-    return () -> object.get(attributeNameValue);
+    return new ResolvedAttribute(object.get(attributeNameValue));
   }
 
   private Object cast(Number number,
diff --git a/src/main/java/com/mitchellbosecke/pebble/attributes/ResolvedAttribute.java b/src/main/java/com/mitchellbosecke/pebble/attributes/ResolvedAttribute.java
index fdc63ec2..236b8162 100644
--- a/src/main/java/com/mitchellbosecke/pebble/attributes/ResolvedAttribute.java
+++ b/src/main/java/com/mitchellbosecke/pebble/attributes/ResolvedAttribute.java
@@ -1,5 +1,9 @@
 package com.mitchellbosecke.pebble.attributes;
 
-public interface ResolvedAttribute {
-  Object evaluate();
+public final class ResolvedAttribute {
+  public final Object evaluatedValue;
+
+  public ResolvedAttribute(Object evaluatedValue) {
+    this.evaluatedValue = evaluatedValue;
+  }
 }
diff --git a/src/main/java/com/mitchellbosecke/pebble/node/expression/GetAttributeExpression.java b/src/main/java/com/mitchellbosecke/pebble/node/expression/GetAttributeExpression.java
index c4e8cbaf..cf6a2782 100644
--- a/src/main/java/com/mitchellbosecke/pebble/node/expression/GetAttributeExpression.java
+++ b/src/main/java/com/mitchellbosecke/pebble/node/expression/GetAttributeExpression.java
@@ -78,7 +78,7 @@ public class GetAttributeExpression implements Expression<Object> {
         for (AttributeResolver attributeResolver: context.getExtensionRegistry().getAttributeResolver()) {
             ResolvedAttribute resolvedAttribute = attributeResolver.resolve(object, attributeNameValue, argumentValues, this.args, context, this.filename, this.lineNumber);
             if (resolvedAttribute != null) {
-                return resolvedAttribute.evaluate();
+                return resolvedAttribute.evaluatedValue;
             }
         }
 
diff --git a/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java b/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
index d9ddfc90..3cd54b90 100644
--- a/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
+++ b/src/main/java/com/mitchellbosecke/pebble/template/ScopeChain.java
@@ -112,19 +112,28 @@ public class ScopeChain {
          * null values must not be handled as "not present".
          */
         Scope scope = this.stack.getFirst();
-        if (scope.containsKey(key)) {
-            return scope.get(key);
+        Object result = scope.get(key);
+        if (result != null) {
+            return result;
         }
 
-        Iterator<Scope> iterator = this.stack.iterator();
-        // account for the first lookup we did
-        iterator.next();
-
-        while (iterator.hasNext()) {
-            scope = iterator.next();
-
+        if (this.stack.size() > 1) {
             if (scope.containsKey(key)) {
-                return scope.get(key);
+                // key could be defined with null and override another value below in the stack
+                return null;
+            }
+            Iterator<Scope> iterator = this.stack.iterator();
+            // account for the first lookup we did
+            iterator.next();
+
+            while (iterator.hasNext()) {
+                result = iterator.next().get(key);
+                if (result != null) {
+                    return result;
+                } else if (scope.containsKey(key)) {
+                    // null value
+                    return null;
+                }
             }
         }
 


Test patch:
diff --git a/src/test/java/com/mitchellbosecke/pebble/ExtendingPebbleTest.java b/src/test/java/com/mitchellbosecke/pebble/ExtendingPebbleTest.java
index 5930028d..43dbc989 100644
--- a/src/test/java/com/mitchellbosecke/pebble/ExtendingPebbleTest.java
+++ b/src/test/java/com/mitchellbosecke/pebble/ExtendingPebbleTest.java
@@ -9,6 +9,7 @@
 package com.mitchellbosecke.pebble;
 
 import com.mitchellbosecke.pebble.attributes.AttributeResolver;
+import com.mitchellbosecke.pebble.attributes.ResolvedAttribute;
 import com.mitchellbosecke.pebble.error.PebbleException;
 import com.mitchellbosecke.pebble.extension.AbstractExtension;
 import com.mitchellbosecke.pebble.extension.Filter;
@@ -98,7 +99,7 @@ public class ExtendingPebbleTest extends AbstractTest {
 
             List<AttributeResolver> attributeResolvers = new ArrayList<>();
             attributeResolvers.add((instance, attribute, argumentValues, args, isStrictVariables, filename, lineNumber) ->
-                    () -> "customAttributeResolver");
+                    new ResolvedAttribute("customAttributeResolver"));
             return attributeResolvers;
         }
     }