Merge pull request eXist-db#6264 from joewiz/feature/performance-optimizations

[optimize] XQuery local-variable resolution and dead cache removal

Author: line-o

exist-core/src/main/java/org/exist/xquery/LocalVariable.java

@@ -35,6 +35,22 @@ public class LocalVariable extends VariableImpl {
 	protected LocalVariable before = null;
 	protected LocalVariable after = null;
 
+	/**
+	 * For O(1) variable resolution: the previous LocalVariable with the same
+	 * {@link #getQName()} that was the topmost binding before this one was
+	 * declared. Used by {@link XQueryContext#popLocalVariables} to restore the
+	 * shadowed binding when this variable goes out of scope.
+	 */
+	LocalVariable prevSameName = null;
+
+	/**
+	 * The {@link #contextStack} marker that was on top when this variable was
+	 * declared. A variable is visible in the current scope iff
+	 * {@code markedUnder == contextStack.peek()}; the previous linked-list
+	 * walk used to express this by stopping at {@code contextStack.peek()}.
+	 */
+	LocalVariable markedUnder = null;
+
     public LocalVariable(QName qname) {
         super(qname);
     }

exist-core/src/main/java/org/exist/xquery/LocationStep.java

@@ -57,9 +57,6 @@ public class LocationStep extends Step {
     protected UpdateListener listener = null;
     protected Expression parent = null;
 
-    // Fields for caching the last result
-    protected CachedResult cached = null;
-
     //private int parentDeps = Dependency.UNKNOWN_DEPENDENCY;
     private boolean preloadedData = false;
     protected boolean optimized = false;
@@ -350,30 +347,6 @@ public Sequence eval(Sequence contextSequence, final Item contextItem)
         if (contextItem != null) {
             contextSequence = contextItem.toSequence();
         }
-        /*
-         * if(contextSequence == null) //Commented because this the high level
-         * result nodeset is *really* null result = NodeSet.EMPTY_SET; //Try to
-         * return cached results else
-         */
-        // TODO: disabled cache for now as it may cause concurrency issues
-        // better use compile-time inspection and maybe a pragma to mark those
-        // sections in the query that can be safely cached
-        // if (cached != null && cached.isValid(contextSequence, contextItem)) {
-        //
-        // // WARNING : commented since predicates are *also* applied below !
-        // // -pb
-        // /*
-        // * if (predicates.size() > 0) { applyPredicate(contextSequence,
-        // * cached.getResult()); } else {
-        // */
-        // result = cached.getResult();
-        // if (context.getProfiler().isEnabled()) {
-        // LOG.debug("Using cached results");
-        // }
-        // context.getProfiler().message(this, Profiler.OPTIMIZATIONS,
-        // "Using cached results", result);
-        //
-        // // }
 
         Sequence result;
         if (needsComputation()) {
@@ -455,11 +428,8 @@ public Sequence eval(Sequence contextSequence, final Item contextItem)
         } else {
             result = NodeSet.EMPTY_SET;
         }
-        // Caches the result
         if (axis != Constants.SELF_AXIS && contextSequence != null
                 && contextSequence.isCacheable()) {
-            // TODO : cache *after* removing duplicates ? -pb
-            cached = new CachedResult(contextSequence, contextItem, result);
             registerUpdateListener();
         }
         // Remove duplicate nodes
@@ -1214,7 +1184,6 @@ protected void registerUpdateListener() {
             listener = new UpdateListener() {
                 @Override
                 public void documentUpdated(final DocumentImpl document, final int event) {
-                    cached = null;
                     if (document == null || event == UpdateListener.ADD || event == UpdateListener.REMOVE) {
                         // clear all
                         currentDocs = null;
@@ -1272,7 +1241,6 @@ public void resetState(final boolean postOptimization) {
             currentSet = null;
             currentDocs = null;
             optimized = false;
-            cached = null;
             listener = null;
         }
     }

exist-core/src/main/java/org/exist/xquery/UserDefinedFunction.java

@@ -28,14 +28,17 @@
 import org.exist.xquery.value.Sequence;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * @author wolf
  */
 public class UserDefinedFunction extends Function implements Cloneable {
 
     private final List<QName> parameters = new ArrayList<>(5);
+    private final Set<QName> parameterNames = new HashSet<>();
     protected boolean visited = false;
     private Expression body;
     private Sequence[] currentArguments = null;
@@ -67,7 +70,7 @@ public void addVariable(final String varName) throws XPathException {
     }
 
     public void addVariable(QName varName) throws XPathException {
-        if (parameters.contains(varName)) {
+        if (!parameterNames.add(varName)) {
             throw new XPathException(this, ErrorCodes.XQST0039, "function " + getName() + " already has a parameter with the name " + varName);
         }
 

exist-core/src/main/java/org/exist/xquery/XQueryContext.java

@@ -187,6 +187,17 @@ public class XQueryContext implements BinaryValueManager, Context {
     // The last element in the linked list of local in-scope variables
     private LocalVariable lastVar = null;
 
+    /**
+     * O(1) lookup table from QName to the most-recently-declared LocalVariable
+     * with that name. Maintained alongside the {@link #lastVar} linked list:
+     * declareVariableBinding adds, popLocalVariables restores the prevSameName
+     * chain. Visibility is enforced in resolveLocalVariable by comparing
+     * {@link LocalVariable#markedUnder} to {@code contextStack.peek()}.
+     */
+    // Shared by reference with copies of this context (see copyFields,
+    // updateContext) just like {@link #contextStack} and {@link #lastVar}.
+    private Map<QName, LocalVariable> localVariableLookup = new HashMap<>();
+
     private Deque<LocalVariable> contextStack = new ArrayDeque<>();
 
     private final Deque<FunctionSignature> callStack = new ArrayDeque<>();
@@ -657,6 +668,7 @@ public void updateContext(final XQueryContext from) {
         this.watchdog = from.watchdog;
         this.lastVar = from.lastVar;
         this.contextStack = from.contextStack;
+        this.localVariableLookup = from.localVariableLookup;
         this.inScopeNamespaces = from.inScopeNamespaces;
         this.inScopePrefixes = from.inScopePrefixes;
         this.inheritedInScopeNamespaces = from.inheritedInScopeNamespaces;
@@ -727,6 +739,7 @@ protected void copyFields(final XQueryContext ctx) {
         ctx.lastVar = this.lastVar;
         ctx.variableStackSize = getCurrentStackSize();
         ctx.contextStack = this.contextStack;
+        ctx.localVariableLookup = this.localVariableLookup;
         ctx.staticNamespaces = new HashMap<>(this.staticNamespaces);
         ctx.staticPrefixes = new HashMap<>(this.staticPrefixes);
 
@@ -1459,6 +1472,7 @@ public void reset(final boolean keepGlobals) {
 
         if (!isShared) {
             lastVar = null;
+            localVariableLookup.clear();
         }
 
         // clear inline functions using closures
@@ -1924,6 +1938,8 @@ public LocalVariable declareVariableBinding(final LocalVariable var) throws XPat
         }
         lastVar = var;
         var.setStackPosition(getCurrentStackSize());
+        var.markedUnder = contextStack.peek();
+        var.prevSameName = localVariableLookup.put(var.getQName(), var);
         return var;
     }
 
@@ -2057,16 +2073,19 @@ Variable resolveGlobalVariable(final QName qname) {
     }
 
     protected Variable resolveLocalVariable(final QName qname) throws XPathException {
-        final LocalVariable end = contextStack.peek();
-        for (LocalVariable var = lastVar; var != null; var = var.before) {
-            if (var == end) {
-                return null;
-            }
-            if (qname.equals(var.getQName())) {
-                return var;
-            }
+        // O(1) fast path. The linked-list walk previously here is O(N) per
+        // call and O(N²) when a body of N variables is analyzed.
+        final LocalVariable var = localVariableLookup.get(qname);
+        if (var == null) {
+            return null;
         }
-        return null;
+        // Visibility: var is visible if it was declared under the current
+        // contextStack mark — the same boundary the linked-list walk used to
+        // express by stopping at {@code contextStack.peek()}.
+        if (var.markedUnder != contextStack.peek()) {
+            return null;
+        }
+        return var;
     }
 
     /**
@@ -2513,10 +2532,27 @@ public void popLocalVariables(@Nullable final LocalVariable var) {
     /**
      * Restore the local variable stack to the position marked by variable var.
      *
+     * <p>Walks {@link #lastVar} backward to {@code var} (or to the start when
+     * {@code var} is {@code null}), unwinding each variable's
+     * {@code prevSameName} chain into {@link #localVariableLookup} in
+     * REVERSE-of-declaration order so that names with multiple bindings in the
+     * popped scope settle on the still-visible binding, not on a popped one.
+     *
      * @param var       only clear variables after this variable, or null
      * @param resultSeq the result sequence
      */
     public void popLocalVariables(@Nullable final LocalVariable var, @Nullable final Sequence resultSeq) {
+        for (LocalVariable cursor = lastVar; cursor != null && cursor != var; cursor = cursor.before) {
+            if (localVariableLookup.get(cursor.getQName()) == cursor) {
+                if (cursor.prevSameName != null) {
+                    localVariableLookup.put(cursor.getQName(), cursor.prevSameName);
+                } else {
+                    localVariableLookup.remove(cursor.getQName());
+                }
+            }
+            cursor.prevSameName = null;
+        }
+
         if (var != null) {
             // clear all variables registered after var. they should be out of scope.
             LocalVariable outOfScope = var.after;