From 68799cb38da7ac67c235674be7dc0fef52f3e056 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 10:40:56 +0200 Subject: [PATCH 01/47] Remove potentially unused code --- .../org/javarosa/core/model/condition/EvaluationContext.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 74bc7d066..c890cc6d4 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -325,10 +325,8 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so TreeReference nodeSetRef = workingRef.clone(); nodeSetRef.add(name, -1); - boolean firstTime = true; List passed = new ArrayList(treeReferences.size()); for (XPathExpression xpe : predicates) { - boolean firstTimeCapture = firstTime; passed.addAll(predicateCache.get(nodeSetRef, xpe, () -> { List predicatePassed = new ArrayList<>(treeReferences.size()); for (int i = 0; i < treeReferences.size(); ++i) { @@ -336,7 +334,7 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so TreeReference treeRef = treeReferences.get(i); //test the predicate on the treeElement - EvaluationContext evalContext = rescope(treeRef, (firstTimeCapture ? treeRef.getMultLast() : i)); + EvaluationContext evalContext = rescope(treeRef, i); Measure.log("PredicateEvaluations"); Object o = xpe.eval(sourceInstance, evalContext); @@ -352,7 +350,6 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so return predicatePassed; })); - firstTime = false; treeReferences.clear(); treeReferences.addAll(passed); passed.clear(); From 5c3520876d455c49c859d837455934a3f48aca53 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 10:46:55 +0200 Subject: [PATCH 02/47] Extract predicate filtering to a method --- .../model/condition/EvaluationContext.java | 55 +++++++++++-------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index c890cc6d4..8a4b5fccd 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -325,34 +325,16 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so TreeReference nodeSetRef = workingRef.clone(); nodeSetRef.add(name, -1); - List passed = new ArrayList(treeReferences.size()); for (XPathExpression xpe : predicates) { - passed.addAll(predicateCache.get(nodeSetRef, xpe, () -> { - List predicatePassed = new ArrayList<>(treeReferences.size()); - for (int i = 0; i < treeReferences.size(); ++i) { - //if there are predicates then we need to see if e.nextElement meets the standard of the predicate - TreeReference treeRef = treeReferences.get(i); - - //test the predicate on the treeElement - EvaluationContext evalContext = rescope(treeRef, i); - - Measure.log("PredicateEvaluations"); - Object o = xpe.eval(sourceInstance, evalContext); - - if (o instanceof Boolean) { - boolean testOutcome = (Boolean) o; - if (testOutcome) { - predicatePassed.add(treeRef); - } - } - } - - return predicatePassed; - })); + List passed = filterWithPredicate( + sourceInstance, + nodeSetRef, + xpe, + treeReferences + ); treeReferences.clear(); treeReferences.addAll(passed); - passed.clear(); if (predicateEvaluationProgress != null) { predicateEvaluationProgress[0]++; @@ -365,6 +347,31 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } } + private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children) { + return predicateCache.get(treeReference, predicate, () -> { + List predicatePassed = new ArrayList<>(children.size()); + for (int i = 0; i < children.size(); ++i) { + //if there are predicates then we need to see if e.nextElement meets the standard of the predicate + TreeReference treeRef = children.get(i); + + //test the predicate on the treeElement + EvaluationContext evalContext = rescope(treeRef, i); + + Measure.log("PredicateEvaluations"); + Object o = predicate.eval(sourceInstance, evalContext); + + if (o instanceof Boolean) { + boolean testOutcome = (Boolean) o; + if (testOutcome) { + predicatePassed.add(treeRef); + } + } + } + + return predicatePassed; + }); + } + private EvaluationContext rescope(TreeReference treeRef, int currentContextPosition) { EvaluationContext ec = new EvaluationContext(this, treeRef); // broken: From d7c37b2533167aec95060841a810e7652887c734 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 11:19:32 +0200 Subject: [PATCH 03/47] Use chained approach for caching implemenation --- .../model/IdempotentInMemPredicateCache.java | 32 +++++++-------- .../javarosa/core/model/TriggerableDag.java | 10 ++++- .../condition/CachingPredicateFilter.java | 27 +++++++++++++ .../model/condition/EvaluationContext.java | 40 ++++++------------- .../core/model/condition/PredicateCache.java | 10 +---- .../core/model/condition/PredicateFilter.java | 14 +++++++ .../condition/XPathEvalPredicateFilter.java | 40 +++++++++++++++++++ 7 files changed, 118 insertions(+), 55 deletions(-) create mode 100644 src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java create mode 100644 src/main/java/org/javarosa/core/model/condition/PredicateFilter.java create mode 100644 src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java diff --git a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java index d0a243577..75107e2ad 100644 --- a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java +++ b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java @@ -1,37 +1,33 @@ package org.javarosa.core.model; +import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.PredicateCache; +import org.javarosa.core.model.condition.PredicateFilter; +import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.function.Supplier; -/** - * In memory implementation of a {@link PredicateCache}. Cannot cache predicate evaluations that contain a - * non-idempotent function. - */ -public class IdempotentInMemPredicateCache implements PredicateCache { +public class IdempotentInMemPredicateCache implements PredicateCache, PredicateFilter { public Map> cachedEvaluations = new HashMap<>(); + @Nullable @Override - @NotNull - public List get(TreeReference nodeSet, XPathExpression predicate, Supplier> onMiss) { + public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext) { String key = getKey(nodeSet, predicate); + return cachedEvaluations.getOrDefault(key, null); + } - if (cachedEvaluations.containsKey(key)) { - return cachedEvaluations.get(key); - } else { - List references = onMiss.get(); - if (isCacheable(predicate)) { - cachedEvaluations.put(key, references); - } - - return references; + @Override + public void cache(TreeReference nodeSet, XPathExpression predicate, List treeReferences) { + String key = getKey(nodeSet, predicate); + if (isCacheable(predicate)) { + cachedEvaluations.put(key, treeReferences); } } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index a0cbe37a5..64a90e99b 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -16,10 +16,12 @@ package org.javarosa.core.model; +import org.javarosa.core.model.condition.CachingPredicateFilter; import org.javarosa.core.model.condition.Condition; import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.Recalculate; import org.javarosa.core.model.condition.Triggerable; +import org.javarosa.core.model.condition.XPathEvalPredicateFilter; import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.TreeElement; @@ -39,6 +41,7 @@ import java.io.DataOutputStream; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -516,9 +519,14 @@ private Set getAllToTrigger(Set cascadeRoots private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext evalContext, Set toTrigger, TreeReference changedRef, Set affectAllRepeatInstances, Set alreadyEvaluated) { Set evaluated = new HashSet<>(); + EvaluationContext context; if (predicateCaching) { - context = new EvaluationContext(evalContext, new IdempotentInMemPredicateCache()); + IdempotentInMemPredicateCache cache = new IdempotentInMemPredicateCache(); + context = new EvaluationContext(evalContext, Arrays.asList( + cache, + new CachingPredicateFilter(cache, new XPathEvalPredicateFilter()) + )); } else { context = evalContext; } diff --git a/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java new file mode 100644 index 000000000..b505f84f6 --- /dev/null +++ b/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java @@ -0,0 +1,27 @@ +package org.javarosa.core.model.condition; + +import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.xpath.expr.XPathExpression; +import org.jetbrains.annotations.Nullable; + +import java.util.List; + +public class CachingPredicateFilter implements PredicateFilter { + + private final PredicateCache cache; + private final PredicateFilter filter; + + public CachingPredicateFilter(PredicateCache cache, PredicateFilter filter) { + this.cache = cache; + this.filter = filter; + } + + @Nullable + @Override + public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext) { + List filtered = filter.filter(sourceInstance, treeReference, predicate, children, evaluationContext); + cache.cache(treeReference, predicate, filtered); + return filtered; + } +} diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 8a4b5fccd..977f17f66 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -21,7 +21,6 @@ import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeElement; import org.javarosa.core.model.instance.TreeReference; -import org.javarosa.measure.Measure; import org.javarosa.xpath.IExprDataType; import org.javarosa.xpath.expr.XPathExpression; @@ -30,6 +29,8 @@ import java.util.HashMap; import java.util.List; +import static java.util.Collections.singletonList; + /** * A collection of objects that affect the evaluation of an expression, like * function handlers and (not supported) variable bindings. @@ -63,8 +64,7 @@ public class EvaluationContext { private DataInstance instance; private int[] predicateEvaluationProgress; - private PredicateCache predicateCache = ((reference, predicate, onMiss) -> onMiss.get()); - + private List predicateFilterChain = singletonList(new XPathEvalPredicateFilter()); /** * Copy Constructor @@ -91,12 +91,12 @@ private EvaluationContext(EvaluationContext base) { //invalidate this currentContextPosition = base.currentContextPosition; - predicateCache = base.predicateCache; + predicateFilterChain = base.predicateFilterChain; } - public EvaluationContext(EvaluationContext base, PredicateCache predicateCache) { + public EvaluationContext(EvaluationContext base, List predicateFilterChain) { this(base); - this.predicateCache = predicateCache; + this.predicateFilterChain = predicateFilterChain; } public EvaluationContext(EvaluationContext base, TreeReference context) { @@ -348,31 +348,17 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children) { - return predicateCache.get(treeReference, predicate, () -> { - List predicatePassed = new ArrayList<>(children.size()); - for (int i = 0; i < children.size(); ++i) { - //if there are predicates then we need to see if e.nextElement meets the standard of the predicate - TreeReference treeRef = children.get(i); - - //test the predicate on the treeElement - EvaluationContext evalContext = rescope(treeRef, i); - - Measure.log("PredicateEvaluations"); - Object o = predicate.eval(sourceInstance, evalContext); - - if (o instanceof Boolean) { - boolean testOutcome = (Boolean) o; - if (testOutcome) { - predicatePassed.add(treeRef); - } - } + for (PredicateFilter predicateFilter : predicateFilterChain) { + List filtered = predicateFilter.filter(sourceInstance, treeReference, predicate, children, this); + if (filtered != null) { + return filtered; } + } - return predicatePassed; - }); + return children; } - private EvaluationContext rescope(TreeReference treeRef, int currentContextPosition) { + public EvaluationContext rescope(TreeReference treeRef, int currentContextPosition) { EvaluationContext ec = new EvaluationContext(this, treeRef); // broken: ec.currentContextPosition = currentContextPosition; diff --git a/src/main/java/org/javarosa/core/model/condition/PredicateCache.java b/src/main/java/org/javarosa/core/model/condition/PredicateCache.java index a074a0d36..9afcefc0b 100644 --- a/src/main/java/org/javarosa/core/model/condition/PredicateCache.java +++ b/src/main/java/org/javarosa/core/model/condition/PredicateCache.java @@ -2,18 +2,10 @@ import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.NotNull; import java.util.List; -import java.util.function.Supplier; -/** - * Allows the result of predicate evaluations (references for matching nodes) to be cached. The cache doesn't know - * anything about the values that might be referenced in a predicate (the "triggerables"), so can only be used in cases - * where the predicates are "static". - */ public interface PredicateCache { - @NotNull - List get(TreeReference nodeSet, XPathExpression predicate, Supplier> onMiss); + void cache(TreeReference nodeSet, XPathExpression predicate, List treeReferences); } diff --git a/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java new file mode 100644 index 000000000..36f808017 --- /dev/null +++ b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java @@ -0,0 +1,14 @@ +package org.javarosa.core.model.condition; + +import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.xpath.expr.XPathExpression; +import org.jetbrains.annotations.Nullable; + +import java.util.List; + +public interface PredicateFilter { + + @Nullable + List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext); +} diff --git a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java new file mode 100644 index 000000000..dc1782de8 --- /dev/null +++ b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java @@ -0,0 +1,40 @@ +package org.javarosa.core.model.condition; + +import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.measure.Measure; +import org.javarosa.xpath.expr.XPathExpression; +import org.jetbrains.annotations.NotNull; + +import java.util.ArrayList; +import java.util.List; + +public class XPathEvalPredicateFilter implements PredicateFilter { + + @NotNull + @Override + public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext) { + List predicatePassed = new ArrayList<>(children.size()); + for (int i = 0; i < children.size(); ++i) { + //if there are predicates then we need to see if e.nextElement meets the standard of the predicate + TreeReference treeRef = children.get(i); + + //test the predicate on the treeElement + EvaluationContext evalContext = evaluationContext.rescope(treeRef, i); + + Measure.log("PredicateEvaluations"); + Object o = predicate.eval(sourceInstance, evalContext); + + if (o instanceof Boolean) { + boolean testOutcome = (Boolean) o; + if (testOutcome) { + predicatePassed.add(treeRef); + } + } + } + + return predicatePassed; + } + + +} From 5b61f2480b6161800377d8e01dc77482dc8e096f Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 11:36:10 +0200 Subject: [PATCH 04/47] Simplify chain with recursive implementation --- .../model/IdempotentInMemPredicateCache.java | 22 ++++++++------- .../javarosa/core/model/TriggerableDag.java | 6 ++--- .../condition/CachingPredicateFilter.java | 27 ------------------- .../model/condition/EvaluationContext.java | 13 +++++---- .../core/model/condition/PredicateCache.java | 11 -------- .../core/model/condition/PredicateFilter.java | 8 +++++- .../condition/XPathEvalPredicateFilter.java | 10 ++++--- 7 files changed, 34 insertions(+), 63 deletions(-) delete mode 100644 src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java delete mode 100644 src/main/java/org/javarosa/core/model/condition/PredicateCache.java diff --git a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java index 75107e2ad..1c67a9c53 100644 --- a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java +++ b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java @@ -1,7 +1,6 @@ package org.javarosa.core.model; import org.javarosa.core.model.condition.EvaluationContext; -import org.javarosa.core.model.condition.PredicateCache; import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; @@ -11,23 +10,26 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Supplier; -public class IdempotentInMemPredicateCache implements PredicateCache, PredicateFilter { +public class IdempotentInMemPredicateCache implements PredicateFilter { public Map> cachedEvaluations = new HashMap<>(); @Nullable @Override - public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext) { + public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { String key = getKey(nodeSet, predicate); - return cachedEvaluations.getOrDefault(key, null); - } - @Override - public void cache(TreeReference nodeSet, XPathExpression predicate, List treeReferences) { - String key = getKey(nodeSet, predicate); - if (isCacheable(predicate)) { - cachedEvaluations.put(key, treeReferences); + if (cachedEvaluations.containsKey(key)) { + return cachedEvaluations.get(key); + } else { + List filtered = next.get(); + if (isCacheable(predicate)) { + cachedEvaluations.put(key, filtered); + } + + return filtered; } } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 64a90e99b..ec094658c 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -16,7 +16,6 @@ package org.javarosa.core.model; -import org.javarosa.core.model.condition.CachingPredicateFilter; import org.javarosa.core.model.condition.Condition; import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.Recalculate; @@ -522,10 +521,9 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext context; if (predicateCaching) { - IdempotentInMemPredicateCache cache = new IdempotentInMemPredicateCache(); context = new EvaluationContext(evalContext, Arrays.asList( - cache, - new CachingPredicateFilter(cache, new XPathEvalPredicateFilter()) + new IdempotentInMemPredicateCache(), + new XPathEvalPredicateFilter() )); } else { context = evalContext; diff --git a/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java deleted file mode 100644 index b505f84f6..000000000 --- a/src/main/java/org/javarosa/core/model/condition/CachingPredicateFilter.java +++ /dev/null @@ -1,27 +0,0 @@ -package org.javarosa.core.model.condition; - -import org.javarosa.core.model.instance.DataInstance; -import org.javarosa.core.model.instance.TreeReference; -import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.Nullable; - -import java.util.List; - -public class CachingPredicateFilter implements PredicateFilter { - - private final PredicateCache cache; - private final PredicateFilter filter; - - public CachingPredicateFilter(PredicateCache cache, PredicateFilter filter) { - this.cache = cache; - this.filter = filter; - } - - @Nullable - @Override - public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext) { - List filtered = filter.filter(sourceInstance, treeReference, predicate, children, evaluationContext); - cache.cache(treeReference, predicate, filtered); - return filtered; - } -} diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 977f17f66..763f1e1b9 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -348,14 +348,13 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children) { - for (PredicateFilter predicateFilter : predicateFilterChain) { - List filtered = predicateFilter.filter(sourceInstance, treeReference, predicate, children, this); - if (filtered != null) { - return filtered; - } - } + return filterWithPredicate(sourceInstance, treeReference, predicate, children, 0); + } - return children; + private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, int i) { + return predicateFilterChain.get(i).filter(sourceInstance, treeReference, predicate, children, this, () -> { + return filterWithPredicate(sourceInstance, treeReference, predicate, children, i + 1); + }); } public EvaluationContext rescope(TreeReference treeRef, int currentContextPosition) { diff --git a/src/main/java/org/javarosa/core/model/condition/PredicateCache.java b/src/main/java/org/javarosa/core/model/condition/PredicateCache.java deleted file mode 100644 index 9afcefc0b..000000000 --- a/src/main/java/org/javarosa/core/model/condition/PredicateCache.java +++ /dev/null @@ -1,11 +0,0 @@ -package org.javarosa.core.model.condition; - -import org.javarosa.core.model.instance.TreeReference; -import org.javarosa.xpath.expr.XPathExpression; - -import java.util.List; - -public interface PredicateCache { - - void cache(TreeReference nodeSet, XPathExpression predicate, List treeReferences); -} diff --git a/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java index 36f808017..4ee0b073d 100644 --- a/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java @@ -6,9 +6,15 @@ import org.jetbrains.annotations.Nullable; import java.util.List; +import java.util.function.Supplier; public interface PredicateFilter { @Nullable - List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext); + List filter(DataInstance sourceInstance, + TreeReference nodeSet, + XPathExpression predicate, + List children, + EvaluationContext evaluationContext, + Supplier> next); } diff --git a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java index dc1782de8..26bd9e3c6 100644 --- a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java @@ -8,12 +8,18 @@ import java.util.ArrayList; import java.util.List; +import java.util.function.Supplier; public class XPathEvalPredicateFilter implements PredicateFilter { @NotNull @Override - public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext) { + public List filter(DataInstance sourceInstance, + TreeReference nodeSet, + XPathExpression predicate, + List children, + EvaluationContext evaluationContext, + Supplier> nextFilter) { List predicatePassed = new ArrayList<>(children.size()); for (int i = 0; i < children.size(); ++i) { //if there are predicates then we need to see if e.nextElement meets the standard of the predicate @@ -35,6 +41,4 @@ public List filter(DataInstance sourceInstance, TreeReference tre return predicatePassed; } - - } From e88f90da1702d1ef4bc0aafb637f74cf6547251e Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 11:39:27 +0200 Subject: [PATCH 05/47] Allow chain to built up rather than set --- .../java/org/javarosa/core/model/TriggerableDag.java | 8 ++------ .../javarosa/core/model/condition/EvaluationContext.java | 9 +++++++-- .../core/model/condition/XPathEvalPredicateFilter.java | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index ec094658c..0fde90ae3 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -20,7 +20,6 @@ import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.Recalculate; import org.javarosa.core.model.condition.Triggerable; -import org.javarosa.core.model.condition.XPathEvalPredicateFilter; import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.TreeElement; @@ -40,7 +39,6 @@ import java.io.DataOutputStream; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -51,6 +49,7 @@ import java.util.Set; import static java.util.Collections.emptySet; +import static java.util.Collections.singletonList; public class TriggerableDag { private static final Logger logger = LoggerFactory.getLogger(TriggerableDag.class); @@ -521,10 +520,7 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext context; if (predicateCaching) { - context = new EvaluationContext(evalContext, Arrays.asList( - new IdempotentInMemPredicateCache(), - new XPathEvalPredicateFilter() - )); + context = new EvaluationContext(evalContext, singletonList(new IdempotentInMemPredicateCache())); } else { context = evalContext; } diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 763f1e1b9..6433dc706 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -28,6 +28,8 @@ import java.util.Date; import java.util.HashMap; import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import static java.util.Collections.singletonList; @@ -94,9 +96,12 @@ private EvaluationContext(EvaluationContext base) { predicateFilterChain = base.predicateFilterChain; } - public EvaluationContext(EvaluationContext base, List predicateFilterChain) { + public EvaluationContext(EvaluationContext base, List aroundPredicateFilterChain) { this(base); - this.predicateFilterChain = predicateFilterChain; + this.predicateFilterChain = Stream.concat( + aroundPredicateFilterChain.stream(), + predicateFilterChain.stream() + ).collect(Collectors.toList()); } public EvaluationContext(EvaluationContext base, TreeReference context) { diff --git a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java index 26bd9e3c6..00af478a3 100644 --- a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java @@ -10,7 +10,7 @@ import java.util.List; import java.util.function.Supplier; -public class XPathEvalPredicateFilter implements PredicateFilter { +class XPathEvalPredicateFilter implements PredicateFilter { @NotNull @Override From beb837fb046a5a7ee0fcf9a5749ca84f77d98967 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 11:43:21 +0200 Subject: [PATCH 06/47] Use lambda instead of anonymous --- .../org/javarosa/core/model/test/PredicateCachingTest.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index fda054c88..be94b7f92 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -24,11 +24,8 @@ public class PredicateCachingTest { public void repeatedPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { Scenario scenario = Scenario.init("secondary-instance-filter.xml"); - int evaluations = Measure.withMeasure("PredicateEvaluations", new Runnable() { - @Override - public void run() { - scenario.answer("/data/choice","a"); - } + int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + scenario.answer("/data/choice", "a"); }); // Check that we do less than (size of secondary instance) * (number of calculates with a filter) From a9cf53f35c8ca50c42eb5f8b89659cdc051c40f9 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 13:32:00 +0200 Subject: [PATCH 07/47] Implement limited between answer predicate caching This only works for predicates of the form: ``` = ``` It's very likely there are untested edge cases that this implementation will break. --- .../model/IdempotentInMemPredicateCache.java | 2 +- .../core/model/IndexingPredicateFilter.java | 47 +++++++++++++++++++ .../javarosa/core/model/TriggerableDag.java | 9 +++- .../java/org/javarosa/measure/Measure.java | 6 ++- .../core/model/test/PredicateCachingTest.java | 15 +++++- 5 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java diff --git a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java index 1c67a9c53..e104ecba0 100644 --- a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java +++ b/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java @@ -14,7 +14,7 @@ public class IdempotentInMemPredicateCache implements PredicateFilter { - public Map> cachedEvaluations = new HashMap<>(); + private final Map> cachedEvaluations = new HashMap<>(); @Nullable @Override diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java new file mode 100644 index 000000000..ca62da747 --- /dev/null +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -0,0 +1,47 @@ +package org.javarosa.core.model; + +import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.condition.PredicateFilter; +import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.xpath.expr.XPathEqExpr; +import org.javarosa.xpath.expr.XPathExpression; +import org.javarosa.xpath.expr.XPathPathExpr; +import org.jetbrains.annotations.Nullable; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; + +public class IndexingPredicateFilter implements PredicateFilter { + + private final Map> cachedEvaluations = new HashMap<>(); + + @Nullable + @Override + public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + String key = null; + if (predicate instanceof XPathEqExpr && + ((XPathEqExpr) predicate).a instanceof XPathPathExpr && + ((XPathEqExpr) predicate).b instanceof XPathPathExpr) { + XPathPathExpr left = (XPathPathExpr) ((XPathEqExpr) predicate).a; + XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; + + Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); + key = treeReference.toString() + left.toString() + rightValue.toString(); + } + + if (key != null) { + if (cachedEvaluations.containsKey(key)) { + return cachedEvaluations.get(key); + } else { + List filtered = next.get(); + cachedEvaluations.put(key, filtered); + return filtered; + } + } else { + return next.get(); + } + } +} diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 0fde90ae3..e1238da14 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -18,6 +18,7 @@ import org.javarosa.core.model.condition.Condition; import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.condition.Recalculate; import org.javarosa.core.model.condition.Triggerable; import org.javarosa.core.model.instance.AbstractTreeElement; @@ -39,6 +40,7 @@ import java.io.DataOutputStream; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -49,7 +51,6 @@ import java.util.Set; import static java.util.Collections.emptySet; -import static java.util.Collections.singletonList; public class TriggerableDag { private static final Logger logger = LoggerFactory.getLogger(TriggerableDag.class); @@ -98,6 +99,7 @@ public interface EventNotifierAccessor { private Map relevancePerRepeat = new HashMap<>(); private boolean predicateCaching = true; + private PredicateFilter predicateIndex = new IndexingPredicateFilter(); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; @@ -520,7 +522,10 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext context; if (predicateCaching) { - context = new EvaluationContext(evalContext, singletonList(new IdempotentInMemPredicateCache())); + context = new EvaluationContext(evalContext, Arrays.asList( + predicateIndex, + new IdempotentInMemPredicateCache() + )); } else { context = evalContext; } diff --git a/src/main/java/org/javarosa/measure/Measure.java b/src/main/java/org/javarosa/measure/Measure.java index c225a5423..a4db5bab6 100644 --- a/src/main/java/org/javarosa/measure/Measure.java +++ b/src/main/java/org/javarosa/measure/Measure.java @@ -42,6 +42,10 @@ private static void stop() { } private static int getCount(String event) { - return counts.get(event); + if (counts.containsKey(event)) { + return counts.get(event); + } else { + return 0; + } } } diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index be94b7f92..87d6203a3 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -21,7 +21,7 @@ public class PredicateCachingTest { @Test - public void repeatedPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { + public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { Scenario scenario = Scenario.init("secondary-instance-filter.xml"); int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { @@ -32,6 +32,19 @@ public void repeatedPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Except assertThat(evaluations, lessThan(4)); } + @Test + public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { + Scenario scenario = Scenario.init("secondary-instance-filter.xml"); + + int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + scenario.answer("/data/choice", "a"); + scenario.answer("/data/choice", "a"); + }); + + // Check that we do less than size of secondary instance * 2 + assertThat(evaluations, lessThan(4)); + } + /** * A form with multiple secondary instances can have expressions with "equivalent" predicates that filter on * different sets of children. It's pretty possible to write a bug where these predicates are treated as the same From dbe239c5382e475c332f2dd31e7af508ac9d699b Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 16:27:56 +0200 Subject: [PATCH 08/47] Add failing test for case where relative expression is on right hand side of eq --- .../core/model/test/PredicateCachingTest.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 87d6203a3..efee38ace 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -109,4 +109,42 @@ public void predicatesOnDifferentChildNamesDoNotGetConfused() throws Exception { assertThat(scenario.answerOf("/data/cat").getValue(), equalTo("12")); assertThat(scenario.answerOf("/data/dog").getValue(), equalTo("9")); } + + @Test + public void eqExpressionsWorkIfEitherSideIsRelative() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calcltr"), + t("calcrtl"), + t("input") + )), + instance("instance", + t("item", + t("value", "A") + ), + t("item", + t("value", "B") + ) + ), + bind("/data/calcltr").type("string") + .calculate("instance('instance')/root/item[value = /data/input]/value"), + bind("/data/calcrtl").type("string") + .calculate("instance('instance')/root/item[/data/input = value]/value"), + bind("/data/input").type("string") + ) + ), + body(input("/data/input")) + )); + + scenario.answer("/data/input", "A"); + assertThat(scenario.answerOf("/data/calcltr").getValue(), equalTo("A")); + assertThat(scenario.answerOf("/data/calcrtl").getValue(), equalTo("A")); + + scenario.answer("/data/input", "B"); + assertThat(scenario.answerOf("/data/calcltr").getValue(), equalTo("B")); + assertThat(scenario.answerOf("/data/calcrtl").getValue(), equalTo("B")); + } } From b069f4c656154c3f7b4c1a47e763c1f6a3f8ebc8 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 11 Apr 2023 17:13:47 +0200 Subject: [PATCH 09/47] Only allow expressions where the left side is relative --- .../java/org/javarosa/core/model/IndexingPredicateFilter.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java index ca62da747..68e56cb97 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -24,7 +24,8 @@ public List filter(DataInstance sourceInstance, TreeReference tre String key = null; if (predicate instanceof XPathEqExpr && ((XPathEqExpr) predicate).a instanceof XPathPathExpr && - ((XPathEqExpr) predicate).b instanceof XPathPathExpr) { + ((XPathEqExpr) predicate).b instanceof XPathPathExpr && + ((XPathPathExpr) ((XPathEqExpr) predicate).a).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { XPathPathExpr left = (XPathPathExpr) ((XPathEqExpr) predicate).a; XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; From c8f0d1f8cc297179632d0b0538f7044a3deb019c Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 12 Apr 2023 12:27:53 +0200 Subject: [PATCH 10/47] Rename param --- .../java/org/javarosa/core/model/IndexingPredicateFilter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java index 68e56cb97..b36fa302b 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -20,7 +20,7 @@ public class IndexingPredicateFilter implements PredicateFilter { @Nullable @Override - public List filter(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { String key = null; if (predicate instanceof XPathEqExpr && ((XPathEqExpr) predicate).a instanceof XPathPathExpr && @@ -30,7 +30,7 @@ public List filter(DataInstance sourceInstance, TreeReference tre XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); - key = treeReference.toString() + left.toString() + rightValue.toString(); + key = nodeSet.toString() + left.toString() + rightValue.toString(); } if (key != null) { From a8dc7bd954c88a5c95ac86fbf26fbb316f45dcc4 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 12 Apr 2023 12:32:19 +0200 Subject: [PATCH 11/47] Correct test form --- src/test/resources/secondary-instance-filter.xml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/resources/secondary-instance-filter.xml b/src/test/resources/secondary-instance-filter.xml index f541b5190..69ad4d9ad 100644 --- a/src/test/resources/secondary-instance-filter.xml +++ b/src/test/resources/secondary-instance-filter.xml @@ -7,7 +7,8 @@ - + + @@ -24,9 +25,9 @@ + nodeset="/data/calculate1" type="string" /> + nodeset="/data/calculate2" type="string" /> From 890454dcaf7f781ed68be9626b9625bca315c48c Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 12 Apr 2023 18:22:30 +0200 Subject: [PATCH 12/47] Add stub for potentially failing case --- .../org/javarosa/core/model/test/PredicateCachingTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index efee38ace..6286d87ea 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -17,6 +17,7 @@ import static org.javarosa.core.util.XFormsElement.model; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; +import static org.junit.Assert.fail; public class PredicateCachingTest { @@ -147,4 +148,9 @@ public void eqExpressionsWorkIfEitherSideIsRelative() throws Exception { assertThat(scenario.answerOf("/data/calcltr").getValue(), equalTo("B")); assertThat(scenario.answerOf("/data/calcrtl").getValue(), equalTo("B")); } + + @Test + public void eqExpressionsWorkIfBothSidesAreRelative() { + fail(); + } } From 8447ba37df0c05d5c6d3d974e5735400ccfc6250 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 09:13:40 +0200 Subject: [PATCH 13/47] Add failing test for cmatching predicates that come after different ones --- .../core/model/test/PredicateCachingTest.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 6286d87ea..2dbbb9018 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -153,4 +153,46 @@ public void eqExpressionsWorkIfEitherSideIsRelative() throws Exception { public void eqExpressionsWorkIfBothSidesAreRelative() { fail(); } + + @Test + public void predicatesInMultipleSetsDoNotGetConfused() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calc"), + t("calc2"), + t("input") + )), + instance("instance", + t("item", + t("value", "A"), + t("count", "2"), + t("id", "A2") + ), + t("item", + t("value", "A"), + t("count", "3"), + t("id", "A3") + ), + t("item", + t("value", "B"), + t("count", "2"), + t("id", "B2") + ) + ), + bind("/data/calc").type("string") + .calculate("instance('instance')/root/item[value = 'A'][count = '3']/id"), + bind("/data/calc2").type("string") + .calculate("instance('instance')/root/item[value = 'B'][count = '3']/id"), + bind("/data/input").type("string") + ) + ), + body(input("/data/input")) + )); + + assertThat(scenario.answerOf("/data/calc").getValue(), equalTo("A3")); + assertThat(scenario.answerOf("/data/calc2"), equalTo(null)); + } } From 5884aebcf585611e0b7ec7a255561c46b7e08e0a Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 14:01:23 +0200 Subject: [PATCH 14/47] Add check for potential regression --- .../core/model/test/PredicateCachingTest.java | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 2dbbb9018..dc9f84d45 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -17,7 +17,6 @@ import static org.javarosa.core.util.XFormsElement.model; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; -import static org.junit.Assert.fail; public class PredicateCachingTest { @@ -150,8 +149,29 @@ public void eqExpressionsWorkIfEitherSideIsRelative() throws Exception { } @Test - public void eqExpressionsWorkIfBothSidesAreRelative() { - fail(); + public void eqExpressionsWorkIfBothSidesAreRelative() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calc"), + t("input") + )), + instance("instance", + t("item", + t("value", "A") + ) + ), + bind("/data/calc").type("string") + .calculate("instance('instance')/root/item[value = value]/value"), + bind("/data/input").type("string") + ) + ), + body(input("/data/input")) + )); + + assertThat(scenario.answerOf("/data/calc").getValue(), equalTo("A")); } @Test From d50cbfc3c5fc22b2dc7354ac83d1b501fe3cab24 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 14:13:25 +0200 Subject: [PATCH 15/47] Disable predicate caching for multiple predicates for the moment --- .../model/condition/EvaluationContext.java | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 6433dc706..eb4a4f018 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -330,12 +330,20 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so TreeReference nodeSetRef = workingRef.clone(); nodeSetRef.add(name, -1); + List filterChain; + if (predicates.size() == 1) { + filterChain = predicateFilterChain; + } else { + filterChain = singletonList(new XPathEvalPredicateFilter()); + } + for (XPathExpression xpe : predicates) { List passed = filterWithPredicate( sourceInstance, nodeSetRef, xpe, - treeReferences + treeReferences, + filterChain ); treeReferences.clear(); @@ -352,13 +360,13 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } } - private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children) { - return filterWithPredicate(sourceInstance, treeReference, predicate, children, 0); + private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, List filterChain) { + return filterWithPredicate(sourceInstance, treeReference, predicate, children, 0, filterChain); } - private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, int i) { - return predicateFilterChain.get(i).filter(sourceInstance, treeReference, predicate, children, this, () -> { - return filterWithPredicate(sourceInstance, treeReference, predicate, children, i + 1); + private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, int i, List filterChain) { + return filterChain.get(i).filter(sourceInstance, treeReference, predicate, children, this, () -> { + return filterWithPredicate(sourceInstance, treeReference, predicate, children, i + 1, filterChain); }); } From de743ff35f66e6ab27a489af9c77c7296c0a4b91 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 14:16:36 +0200 Subject: [PATCH 16/47] Add stub failing tests for other scenarios we want to make sure are optimized --- .../core/model/test/PredicateCachingTest.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index dc9f84d45..c0ff4efe5 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -17,6 +17,7 @@ import static org.javarosa.core.util.XFormsElement.model; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; +import static org.junit.Assert.fail; public class PredicateCachingTest { @@ -32,6 +33,16 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exce assertThat(evaluations, lessThan(4)); } + @Test + public void repeatedCompPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { + fail(); + } + + @Test + public void repeatedIdempotentFuncPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { + fail(); + } + @Test public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { Scenario scenario = Scenario.init("secondary-instance-filter.xml"); @@ -41,10 +52,15 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { scenario.answer("/data/choice", "a"); }); - // Check that we do less than size of secondary instance * 2 + // Check that we do less than size of secondary instance * number of times we answer assertThat(evaluations, lessThan(4)); } + @Test + public void repeatedCompPredicatesAreOnlyEvaluatedOnce() throws Exception { + fail(); + } + /** * A form with multiple secondary instances can have expressions with "equivalent" predicates that filter on * different sets of children. It's pretty possible to write a bug where these predicates are treated as the same From fbb0a141b6fa2b41c0a63b81eeecbbe749058a89 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 16:22:11 +0200 Subject: [PATCH 17/47] Rewrite tests with Scenario DSL --- .../core/model/test/PredicateCachingTest.java | 56 ++++++++++++++++++- .../resources/secondary-instance-filter.xml | 42 -------------- 2 files changed, 54 insertions(+), 44 deletions(-) delete mode 100644 src/test/resources/secondary-instance-filter.xml diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index c0ff4efe5..7910652f6 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -13,8 +13,10 @@ import static org.javarosa.core.util.XFormsElement.html; import static org.javarosa.core.util.XFormsElement.input; import static org.javarosa.core.util.XFormsElement.instance; +import static org.javarosa.core.util.XFormsElement.item; import static org.javarosa.core.util.XFormsElement.mainInstance; import static org.javarosa.core.util.XFormsElement.model; +import static org.javarosa.core.util.XFormsElement.select1; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; import static org.junit.Assert.fail; @@ -23,7 +25,32 @@ public class PredicateCachingTest { @Test public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { - Scenario scenario = Scenario.init("secondary-instance-filter.xml"); + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("a", "A"), + item("b", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value = /data/choice]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[value = /data/choice]/value") + ) + ), + body( + select1("/data/choice", + item("a", "A") + ) + ) + )); int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { scenario.answer("/data/choice", "a"); @@ -45,7 +72,32 @@ public void repeatedIdempotentFuncPredicatesAreOnlyEvaluatedOnceWhileAnswering() @Test public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { - Scenario scenario = Scenario.init("secondary-instance-filter.xml"); + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("a", "A"), + item("b", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value = /data/choice]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[value = /data/choice]/value") + ) + ), + body( + select1("/data/choice", + item("a", "A") + ) + ) + )); int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { scenario.answer("/data/choice", "a"); diff --git a/src/test/resources/secondary-instance-filter.xml b/src/test/resources/secondary-instance-filter.xml deleted file mode 100644 index 69ad4d9ad..000000000 --- a/src/test/resources/secondary-instance-filter.xml +++ /dev/null @@ -1,42 +0,0 @@ - - - - Secondary instance filter - - - - - - - - - - - - - a - - - - b - - - - - - - - - - - - - - - - - From 4816c1b2edf0cf4b6409c3d10d037eb378ee5251 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 16:27:55 +0200 Subject: [PATCH 18/47] Add passing test for comparison case --- .../core/model/test/PredicateCachingTest.java | 41 +++++++++++++++---- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 7910652f6..fe70e30cb 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -16,7 +16,6 @@ import static org.javarosa.core.util.XFormsElement.item; import static org.javarosa.core.util.XFormsElement.mainInstance; import static org.javarosa.core.util.XFormsElement.model; -import static org.javarosa.core.util.XFormsElement.select1; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; import static org.junit.Assert.fail; @@ -46,9 +45,7 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exce ) ), body( - select1("/data/choice", - item("a", "A") - ) + input("/data/choice") ) )); @@ -62,7 +59,37 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exce @Test public void repeatedCompPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { - fail(); + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("1", "A"), + item("2", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value < /data/choice]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[value < /data/choice]/value") + ) + ), + body( + input("/data/choice") + ) + )); + + int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + scenario.answer("/data/choice", "2"); + }); + + // Check that we do less than (size of secondary instance) * (number of calculates with a filter) + assertThat(evaluations, lessThan(4)); } @Test @@ -93,9 +120,7 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { ) ), body( - select1("/data/choice", - item("a", "A") - ) + input("/data/choice") ) )); From ab7226f6e51e43db569b92d2da8e775d3d7df4bc Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 16:31:48 +0200 Subject: [PATCH 19/47] Add test for failing comp case --- .../core/model/test/PredicateCachingTest.java | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index fe70e30cb..49c1ccd34 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -135,7 +135,38 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { @Test public void repeatedCompPredicatesAreOnlyEvaluatedOnce() throws Exception { - fail(); + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("1", "A"), + item("2", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value < /data/choice]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[value < /data/choice]/value") + ) + ), + body( + input("/data/choice") + ) + )); + + int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + scenario.answer("/data/choice", "2"); + scenario.answer("/data/choice", "2"); + }); + + // Check that we do less than size of secondary instance * number of times we answer + assertThat(evaluations, lessThan(4)); } /** From fc9b983f8494d30b2bc8826fcd75ec9820f39f37 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 13 Apr 2023 16:50:46 +0200 Subject: [PATCH 20/47] Support caching for cmp predicates --- .../core/model/IndexingPredicateFilter.java | 21 ++++++++---- .../core/model/test/PredicateCachingTest.java | 32 +++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java index b36fa302b..7b693ca9e 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -4,6 +4,7 @@ import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; import org.javarosa.xpath.expr.XPathPathExpr; @@ -21,16 +22,24 @@ public class IndexingPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - String key = null; + XPathPathExpr left = null; + XPathPathExpr right = null; if (predicate instanceof XPathEqExpr && ((XPathEqExpr) predicate).a instanceof XPathPathExpr && - ((XPathEqExpr) predicate).b instanceof XPathPathExpr && - ((XPathPathExpr) ((XPathEqExpr) predicate).a).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { - XPathPathExpr left = (XPathPathExpr) ((XPathEqExpr) predicate).a; - XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; + ((XPathEqExpr) predicate).b instanceof XPathPathExpr) { + left = (XPathPathExpr) ((XPathEqExpr) predicate).a; + right = (XPathPathExpr) ((XPathEqExpr) predicate).b; + } else if (predicate instanceof XPathCmpExpr && + ((XPathCmpExpr) predicate).a instanceof XPathPathExpr && + ((XPathCmpExpr) predicate).b instanceof XPathPathExpr) { + left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; + right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; + } + String key = null; + if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); - key = nodeSet.toString() + left.toString() + rightValue.toString(); + key = nodeSet.toString() + predicate + left + rightValue.toString(); } if (key != null) { diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 49c1ccd34..e0231c895 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -339,4 +339,36 @@ public void predicatesInMultipleSetsDoNotGetConfused() throws Exception { assertThat(scenario.answerOf("/data/calc").getValue(), equalTo("A3")); assertThat(scenario.answerOf("/data/calc2"), equalTo(null)); } + + @Test + public void similarCmpAndEqExpressionsDoNotGetConfused() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("input"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("1", "A"), + item("2", "B") + ), + bind("/data/input").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value < /data/input]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[value = /data/input]/label") + ) + ), + body( + input("/data/input") + ) + )); + + scenario.answer("/data/input", "2"); + assertThat(scenario.answerOf("/data/calculate1").getValue(), equalTo("A")); + assertThat(scenario.answerOf("/data/calculate2").getValue(), equalTo("B")); + } } From ae070b560b38df404c15c1474e37c71ab1d6974d Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 09:19:58 +0200 Subject: [PATCH 21/47] Add test for func case --- .../core/model/test/PredicateCachingTest.java | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index e0231c895..a447c0248 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -18,7 +18,6 @@ import static org.javarosa.core.util.XFormsElement.model; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; -import static org.junit.Assert.fail; public class PredicateCachingTest { @@ -94,7 +93,37 @@ public void repeatedCompPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Ex @Test public void repeatedIdempotentFuncPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exception { - fail(); + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("1", "A"), + item("2", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[regex(value, /data/choice)]/label"), + bind("/data/calculate2").type("string") + .calculate("instance('instance')/root/item[regex(value, /data/choice)]/value") + ) + ), + body( + input("/data/choice") + ) + )); + + int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + scenario.answer("/data/choice", "1"); + }); + + // Check that we do less than (size of secondary instance) * (number of calculates with a filter) + assertThat(evaluations, lessThan(4)); } @Test From b543f38d80546dd2b781c693956ea102f8bb28fd Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 10:43:43 +0200 Subject: [PATCH 22/47] Evaluate eq predicates with index --- .../core/model/IndexingPredicateFilter.java | 76 ++++++++++++++----- .../condition/XPathEvalPredicateFilter.java | 2 +- .../java/org/javarosa/measure/Measure.java | 15 ++++ .../core/model/test/PredicateCachingTest.java | 49 ++++++++++-- 4 files changed, 115 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java index 7b693ca9e..7b2118c36 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -1,15 +1,18 @@ package org.javarosa.core.model; +import kotlin.Pair; import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.measure.Measure; import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; import org.javarosa.xpath.expr.XPathPathExpr; import org.jetbrains.annotations.Nullable; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -17,41 +20,74 @@ public class IndexingPredicateFilter implements PredicateFilter { - private final Map> cachedEvaluations = new HashMap<>(); + private final Map> cachedCmpEvaluations = new HashMap<>(); + private final Map, Map>> instanceEqIndexes = new HashMap<>(); @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - XPathPathExpr left = null; - XPathPathExpr right = null; if (predicate instanceof XPathEqExpr && ((XPathEqExpr) predicate).a instanceof XPathPathExpr && - ((XPathEqExpr) predicate).b instanceof XPathPathExpr) { - left = (XPathPathExpr) ((XPathEqExpr) predicate).a; - right = (XPathPathExpr) ((XPathEqExpr) predicate).b; + ((XPathEqExpr) predicate).b instanceof XPathPathExpr && + ((XPathPathExpr) ((XPathEqExpr) predicate).a).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE && + ((XPathPathExpr) ((XPathEqExpr) predicate).b).init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { + XPathPathExpr left = (XPathPathExpr) ((XPathEqExpr) predicate).a; + Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), left.toString()); + if (!instanceEqIndexes.containsKey(indexKey)) { + instanceEqIndexes.put(indexKey, new HashMap<>()); + } + + Map> index = instanceEqIndexes.get(indexKey); + if (index.isEmpty()) { + buildEqIndex(sourceInstance, (XPathEqExpr) predicate, children, evaluationContext, index); + } + + XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; + String rightValue = (String) right.eval(sourceInstance, evaluationContext).unpack(); + return index.getOrDefault(rightValue, new ArrayList<>()); } else if (predicate instanceof XPathCmpExpr && ((XPathCmpExpr) predicate).a instanceof XPathPathExpr && ((XPathCmpExpr) predicate).b instanceof XPathPathExpr) { - left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; - right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; - } + XPathPathExpr left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; + XPathPathExpr right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; - String key = null; - if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { - Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); - key = nodeSet.toString() + predicate + left + rightValue.toString(); - } + String key = null; + if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { + Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); + key = nodeSet.toString() + predicate + left + rightValue.toString(); + } - if (key != null) { - if (cachedEvaluations.containsKey(key)) { - return cachedEvaluations.get(key); + if (key != null) { + if (cachedCmpEvaluations.containsKey(key)) { + return cachedCmpEvaluations.get(key); + } else { + List filtered = next.get(); + cachedCmpEvaluations.put(key, filtered); + return filtered; + } } else { - List filtered = next.get(); - cachedEvaluations.put(key, filtered); - return filtered; + return next.get(); } } else { return next.get(); } } + + private static void buildEqIndex(DataInstance sourceInstance, XPathEqExpr predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { + for (int i = 0; i < children.size(); i++) { + TreeReference child = children.get(i); + XPathPathExpr left = (XPathPathExpr) predicate.a; + + EvaluationContext evalContext = evaluationContext.rescope(child, i); + + Measure.log("IndexEvaluation"); + String leftVal = (String) left.eval(sourceInstance, evalContext).unpack(); + + if (!eqIndex.containsKey(left.toString())) { + eqIndex.put(leftVal, new ArrayList<>()); + } + + eqIndex.get(leftVal).add(child); + } + } } diff --git a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java index 00af478a3..1f23f3880 100644 --- a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java @@ -28,7 +28,7 @@ public List filter(DataInstance sourceInstance, //test the predicate on the treeElement EvaluationContext evalContext = evaluationContext.rescope(treeRef, i); - Measure.log("PredicateEvaluations"); + Measure.log("PredicateEvaluation"); Object o = predicate.eval(sourceInstance, evalContext); if (o instanceof Boolean) { diff --git a/src/main/java/org/javarosa/measure/Measure.java b/src/main/java/org/javarosa/measure/Measure.java index a4db5bab6..1006d9055 100644 --- a/src/main/java/org/javarosa/measure/Measure.java +++ b/src/main/java/org/javarosa/measure/Measure.java @@ -1,6 +1,7 @@ package org.javarosa.measure; import java.util.HashMap; +import java.util.List; import java.util.Map; public class Measure { @@ -21,6 +22,20 @@ public static int withMeasure(String event, Runnable work) { return count; } + public static int withMeasure(List events, Runnable work) { + start(); + work.run(); + + int count = 0; + for (String event : events) { + count += getCount(event); + } + + start(); + + return count; + } + public static void log(String event) { if (!measuring) return; diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index a447c0248..944d0b5c6 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -4,6 +4,7 @@ import org.javarosa.measure.Measure; import org.junit.Test; +import static java.util.Arrays.asList; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.lessThan; @@ -48,7 +49,7 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Exce ) )); - int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { scenario.answer("/data/choice", "a"); }); @@ -83,7 +84,7 @@ public void repeatedCompPredicatesAreOnlyEvaluatedOnceWhileAnswering() throws Ex ) )); - int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { scenario.answer("/data/choice", "2"); }); @@ -118,7 +119,7 @@ public void repeatedIdempotentFuncPredicatesAreOnlyEvaluatedOnceWhileAnswering() ) )); - int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { scenario.answer("/data/choice", "1"); }); @@ -153,9 +154,9 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { ) )); - int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { - scenario.answer("/data/choice", "a"); + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { scenario.answer("/data/choice", "a"); + scenario.answer("/data/choice", "b"); }); // Check that we do less than size of secondary instance * number of times we answer @@ -189,7 +190,7 @@ public void repeatedCompPredicatesAreOnlyEvaluatedOnce() throws Exception { ) )); - int evaluations = Measure.withMeasure("PredicateEvaluations", () -> { + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { scenario.answer("/data/choice", "2"); scenario.answer("/data/choice", "2"); }); @@ -400,4 +401,40 @@ public void similarCmpAndEqExpressionsDoNotGetConfused() throws Exception { assertThat(scenario.answerOf("/data/calculate1").getValue(), equalTo("A")); assertThat(scenario.answerOf("/data/calculate2").getValue(), equalTo("B")); } + + @Test + public void differentEqExpressionsAreNotConfused() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calc1"), + t("calc2"), + t("input1"), + t("input2") + )), + instance("instance", + item("a", "A"), + item("b", "B") + ), + bind("/data/calc1").type("string") + .calculate("instance('instance')/root/item[value = /data/input1]/label"), + bind("/data/calc2").type("string") + .calculate("instance('instance')/root/item[label = /data/input2]/label"), + bind("/data/input").type("string") + ) + ), + body( + input("/data/input1"), + input("/data/input2") + ) + )); + + scenario.answer("/data/input1", "a"); + scenario.answer("/data/input2", "B"); + + assertThat(scenario.answerOf("/data/calc1").getValue(), equalTo("A")); + assertThat(scenario.answerOf("/data/calc2").getValue(), equalTo("B")); + } } From 3399e344833730ec3bd614cb7ffad28f1896d976 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 10:52:31 +0200 Subject: [PATCH 23/47] Split indexing caching out --- .../core/model/CmpCachePredicateFilter.java | 51 +++++++++++++++++++ .../core/model/IndexingPredicateFilter.java | 25 --------- .../javarosa/core/model/TriggerableDag.java | 4 +- 3 files changed, 54 insertions(+), 26 deletions(-) create mode 100644 src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java diff --git a/src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java b/src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java new file mode 100644 index 000000000..e04e7401b --- /dev/null +++ b/src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java @@ -0,0 +1,51 @@ +package org.javarosa.core.model; + +import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.condition.PredicateFilter; +import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.xpath.expr.XPathCmpExpr; +import org.javarosa.xpath.expr.XPathExpression; +import org.javarosa.xpath.expr.XPathPathExpr; +import org.jetbrains.annotations.Nullable; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Supplier; + +public class CmpCachePredicateFilter implements PredicateFilter { + + private final Map> cachedCmpEvaluations = new HashMap<>(); + + @Nullable + @Override + public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + if (predicate instanceof XPathCmpExpr && + ((XPathCmpExpr) predicate).a instanceof XPathPathExpr && + ((XPathCmpExpr) predicate).b instanceof XPathPathExpr) { + XPathPathExpr left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; + XPathPathExpr right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; + + String key = null; + if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { + Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); + key = nodeSet.toString() + predicate + left + rightValue.toString(); + } + + if (key != null) { + if (cachedCmpEvaluations.containsKey(key)) { + return cachedCmpEvaluations.get(key); + } else { + List filtered = next.get(); + cachedCmpEvaluations.put(key, filtered); + return filtered; + } + } else { + return next.get(); + } + } else { + return next.get(); + } + } +} diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java index 7b2118c36..6cfd4abe3 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java @@ -6,7 +6,6 @@ import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; import org.javarosa.measure.Measure; -import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; import org.javarosa.xpath.expr.XPathPathExpr; @@ -20,7 +19,6 @@ public class IndexingPredicateFilter implements PredicateFilter { - private final Map> cachedCmpEvaluations = new HashMap<>(); private final Map, Map>> instanceEqIndexes = new HashMap<>(); @Nullable @@ -45,29 +43,6 @@ public List filter(DataInstance sourceInstance, TreeReference nod XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; String rightValue = (String) right.eval(sourceInstance, evaluationContext).unpack(); return index.getOrDefault(rightValue, new ArrayList<>()); - } else if (predicate instanceof XPathCmpExpr && - ((XPathCmpExpr) predicate).a instanceof XPathPathExpr && - ((XPathCmpExpr) predicate).b instanceof XPathPathExpr) { - XPathPathExpr left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; - XPathPathExpr right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; - - String key = null; - if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { - Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); - key = nodeSet.toString() + predicate + left + rightValue.toString(); - } - - if (key != null) { - if (cachedCmpEvaluations.containsKey(key)) { - return cachedCmpEvaluations.get(key); - } else { - List filtered = next.get(); - cachedCmpEvaluations.put(key, filtered); - return filtered; - } - } else { - return next.get(); - } } else { return next.get(); } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index e1238da14..eac797739 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -99,7 +99,8 @@ public interface EventNotifierAccessor { private Map relevancePerRepeat = new HashMap<>(); private boolean predicateCaching = true; - private PredicateFilter predicateIndex = new IndexingPredicateFilter(); + private final PredicateFilter cmpCache = new CmpCachePredicateFilter(); + private final PredicateFilter predicateIndex = new IndexingPredicateFilter(); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; @@ -524,6 +525,7 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, if (predicateCaching) { context = new EvaluationContext(evalContext, Arrays.asList( predicateIndex, + cmpCache, new IdempotentInMemPredicateCache() )); } else { From aa9bcf2a5241d86345b1dc93462c04e5fe3556c3 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 11:17:36 +0200 Subject: [PATCH 24/47] Rename classes --- ...cateFilter.java => CachingPredicateFilter.java} | 14 +++++++++----- ...ateCache.java => IdempotentPredicateCache.java} | 7 ++++++- ...dicateFilter.java => IndexPredicateFilter.java} | 7 ++++++- .../org/javarosa/core/model/TriggerableDag.java | 10 +++++----- 4 files changed, 26 insertions(+), 12 deletions(-) rename src/main/java/org/javarosa/core/model/{CmpCachePredicateFilter.java => CachingPredicateFilter.java} (77%) rename src/main/java/org/javarosa/core/model/{IdempotentInMemPredicateCache.java => IdempotentPredicateCache.java} (79%) rename src/main/java/org/javarosa/core/model/{IndexingPredicateFilter.java => IndexPredicateFilter.java} (88%) diff --git a/src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java similarity index 77% rename from src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java rename to src/main/java/org/javarosa/core/model/CachingPredicateFilter.java index e04e7401b..ada5efb0e 100644 --- a/src/main/java/org/javarosa/core/model/CmpCachePredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java @@ -14,9 +14,13 @@ import java.util.Map; import java.util.function.Supplier; -public class CmpCachePredicateFilter implements PredicateFilter { +/** + * Caches down stream evaluations (in the {@link PredicateFilter} chain) for supported expressions - currently just + * {@link XPathCmpExpr}. Repeated evaluations are fetched in O(1) time. + */ +public class CachingPredicateFilter implements PredicateFilter { - private final Map> cachedCmpEvaluations = new HashMap<>(); + private final Map> cachedEvaluations = new HashMap<>(); @Nullable @Override @@ -34,11 +38,11 @@ public List filter(DataInstance sourceInstance, TreeReference nod } if (key != null) { - if (cachedCmpEvaluations.containsKey(key)) { - return cachedCmpEvaluations.get(key); + if (cachedEvaluations.containsKey(key)) { + return cachedEvaluations.get(key); } else { List filtered = next.get(); - cachedCmpEvaluations.put(key, filtered); + cachedEvaluations.put(key, filtered); return filtered; } } else { diff --git a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java b/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java similarity index 79% rename from src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java rename to src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java index e104ecba0..ff01a98fa 100644 --- a/src/main/java/org/javarosa/core/model/IdempotentInMemPredicateCache.java +++ b/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java @@ -12,7 +12,12 @@ import java.util.Map; import java.util.function.Supplier; -public class IdempotentInMemPredicateCache implements PredicateFilter { +/** + * Caches down stream evaluations (in the {@link PredicateFilter} chain) for "idempotent" (with respect to current form + * state) predicates. Can only be used for static instances or in cases where form state won't change - will cause + * clashes otherwise. Repeated evaluations are fetched in O(1) time. + */ +public class IdempotentPredicateCache implements PredicateFilter { private final Map> cachedEvaluations = new HashMap<>(); diff --git a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java similarity index 88% rename from src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java rename to src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 6cfd4abe3..76a300a67 100644 --- a/src/main/java/org/javarosa/core/model/IndexingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -17,7 +17,12 @@ import java.util.Map; import java.util.function.Supplier; -public class IndexingPredicateFilter implements PredicateFilter { +/** + * Uses a (lazily constructed) index to evaluate a predicate for supported expressions - currently just + * {@link XPathEqExpr} where one side is relative to the instance child being filtered. Evaluations are fetched in + * O(1) time with O(n) expression evaluations only being required the first time a relative side is evaluated. + */ +public class IndexPredicateFilter implements PredicateFilter { private final Map, Map>> instanceEqIndexes = new HashMap<>(); diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index eac797739..c0f9436e9 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -99,8 +99,8 @@ public interface EventNotifierAccessor { private Map relevancePerRepeat = new HashMap<>(); private boolean predicateCaching = true; - private final PredicateFilter cmpCache = new CmpCachePredicateFilter(); - private final PredicateFilter predicateIndex = new IndexingPredicateFilter(); + private final PredicateFilter cachingPredicateFilter = new CachingPredicateFilter(); + private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; @@ -524,9 +524,9 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext context; if (predicateCaching) { context = new EvaluationContext(evalContext, Arrays.asList( - predicateIndex, - cmpCache, - new IdempotentInMemPredicateCache() + indexPredicateFilter, + cachingPredicateFilter, + new IdempotentPredicateCache() )); } else { context = evalContext; From e559d3ea81452e5d179bb8b70c26b6b2abb24987 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 11:43:32 +0200 Subject: [PATCH 25/47] Pull out expression parsing logic --- .../core/model/CachingPredicateFilter.java | 21 +++---- .../CompareChildToAbsolutePredicate.java | 58 +++++++++++++++++++ .../core/model/IndexPredicateFilter.java | 23 +++----- 3 files changed, 74 insertions(+), 28 deletions(-) create mode 100644 src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java diff --git a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java index ada5efb0e..44a05bf20 100644 --- a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java @@ -5,8 +5,8 @@ import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathCmpExpr; +import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; -import org.javarosa.xpath.expr.XPathPathExpr; import org.jetbrains.annotations.Nullable; import java.util.HashMap; @@ -16,7 +16,7 @@ /** * Caches down stream evaluations (in the {@link PredicateFilter} chain) for supported expressions - currently just - * {@link XPathCmpExpr}. Repeated evaluations are fetched in O(1) time. + * {@link XPathCmpExpr} and {@link XPathEqExpr}. Repeated evaluations are fetched in O(1) time. */ public class CachingPredicateFilter implements PredicateFilter { @@ -25,17 +25,11 @@ public class CachingPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - if (predicate instanceof XPathCmpExpr && - ((XPathCmpExpr) predicate).a instanceof XPathPathExpr && - ((XPathCmpExpr) predicate).b instanceof XPathPathExpr) { - XPathPathExpr left = (XPathPathExpr) ((XPathCmpExpr) predicate).a; - XPathPathExpr right = (XPathPathExpr) ((XPathCmpExpr) predicate).b; - - String key = null; - if (left != null && right != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { - Object rightValue = right.eval(sourceInstance, evaluationContext).unpack(); - key = nodeSet.toString() + predicate + left + rightValue.toString(); - } + CompareChildToAbsolutePredicate candidate = CompareChildToAbsolutePredicate.parse(predicate); + + if (candidate != null) { + Object rightValue = candidate.getAbsoluteSide().eval(sourceInstance, evaluationContext).unpack(); + String key = nodeSet.toString() + predicate + candidate.getRelativeSide() + rightValue.toString(); if (key != null) { if (cachedEvaluations.containsKey(key)) { @@ -52,4 +46,5 @@ public List filter(DataInstance sourceInstance, TreeReference nod return next.get(); } } + } diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java new file mode 100644 index 000000000..b8c6287cc --- /dev/null +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java @@ -0,0 +1,58 @@ +package org.javarosa.core.model; + +import org.javarosa.xpath.expr.XPathCmpExpr; +import org.javarosa.xpath.expr.XPathEqExpr; +import org.javarosa.xpath.expr.XPathExpression; +import org.javarosa.xpath.expr.XPathPathExpr; +import org.jetbrains.annotations.Nullable; + +class CompareChildToAbsolutePredicate { + + private final XPathPathExpr relativeSide; + private final XPathPathExpr absoluteSide; + private final XPathExpression original; + + public CompareChildToAbsolutePredicate(XPathPathExpr relativeSide, XPathPathExpr absoluteSide, XPathExpression original) { + this.relativeSide = relativeSide; + this.absoluteSide = absoluteSide; + this.original = original; + } + + public XPathPathExpr getRelativeSide() { + return relativeSide; + } + + public XPathPathExpr getAbsoluteSide() { + return absoluteSide; + } + + public XPathExpression getOriginal() { + return original; + } + + @Nullable + public static CompareChildToAbsolutePredicate parse(XPathExpression expression) { + XPathPathExpr left = null; + XPathPathExpr right = null; + + if (expression instanceof XPathCmpExpr && + ((XPathCmpExpr) expression).a instanceof XPathPathExpr && + ((XPathCmpExpr) expression).b instanceof XPathPathExpr) { + + left = (XPathPathExpr) ((XPathCmpExpr) expression).a; + right = (XPathPathExpr) ((XPathCmpExpr) expression).b; + } else if (expression instanceof XPathEqExpr && + ((XPathEqExpr) expression).a instanceof XPathPathExpr && + ((XPathEqExpr) expression).b instanceof XPathPathExpr) { + left = (XPathPathExpr) ((XPathEqExpr) expression).a; + right = (XPathPathExpr) ((XPathEqExpr) expression).b; + } + + if (left != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE && + right.init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { + return new CompareChildToAbsolutePredicate(left, right, expression); + } else { + return null; + } + } +} diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 76a300a67..07c6e5937 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -8,7 +8,6 @@ import org.javarosa.measure.Measure; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; -import org.javarosa.xpath.expr.XPathPathExpr; import org.jetbrains.annotations.Nullable; import java.util.ArrayList; @@ -29,41 +28,35 @@ public class IndexPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - if (predicate instanceof XPathEqExpr && - ((XPathEqExpr) predicate).a instanceof XPathPathExpr && - ((XPathEqExpr) predicate).b instanceof XPathPathExpr && - ((XPathPathExpr) ((XPathEqExpr) predicate).a).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE && - ((XPathPathExpr) ((XPathEqExpr) predicate).b).init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { - XPathPathExpr left = (XPathPathExpr) ((XPathEqExpr) predicate).a; - Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), left.toString()); + CompareChildToAbsolutePredicate candidate = CompareChildToAbsolutePredicate.parse(predicate); + if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr) { + Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), candidate.getRelativeSide().toString()); if (!instanceEqIndexes.containsKey(indexKey)) { instanceEqIndexes.put(indexKey, new HashMap<>()); } Map> index = instanceEqIndexes.get(indexKey); if (index.isEmpty()) { - buildEqIndex(sourceInstance, (XPathEqExpr) predicate, children, evaluationContext, index); + buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); } - XPathPathExpr right = (XPathPathExpr) ((XPathEqExpr) predicate).b; - String rightValue = (String) right.eval(sourceInstance, evaluationContext).unpack(); + String rightValue = (String) candidate.getAbsoluteSide().eval(sourceInstance, evaluationContext).unpack(); return index.getOrDefault(rightValue, new ArrayList<>()); } else { return next.get(); } } - private static void buildEqIndex(DataInstance sourceInstance, XPathEqExpr predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { + private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbsolutePredicate predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); - XPathPathExpr left = (XPathPathExpr) predicate.a; EvaluationContext evalContext = evaluationContext.rescope(child, i); Measure.log("IndexEvaluation"); - String leftVal = (String) left.eval(sourceInstance, evalContext).unpack(); + String leftVal = (String) predicate.getRelativeSide().eval(sourceInstance, evalContext).unpack(); - if (!eqIndex.containsKey(left.toString())) { + if (!eqIndex.containsKey(predicate.getRelativeSide().toString())) { eqIndex.put(leftVal, new ArrayList<>()); } From 6785e5a6e55be47ecd3196f39f0f5844fbcc368e Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 12:53:59 +0200 Subject: [PATCH 26/47] Rename class --- .../org/javarosa/core/model/CachingPredicateFilter.java | 2 +- ...edicate.java => CompareChildToAbsoluteExpression.java} | 8 ++++---- .../org/javarosa/core/model/IndexPredicateFilter.java | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) rename src/main/java/org/javarosa/core/model/{CompareChildToAbsolutePredicate.java => CompareChildToAbsoluteExpression.java} (83%) diff --git a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java index 44a05bf20..68aecde13 100644 --- a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java @@ -25,7 +25,7 @@ public class CachingPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - CompareChildToAbsolutePredicate candidate = CompareChildToAbsolutePredicate.parse(predicate); + CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null) { Object rightValue = candidate.getAbsoluteSide().eval(sourceInstance, evaluationContext).unpack(); diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java similarity index 83% rename from src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java rename to src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java index b8c6287cc..da5593f14 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsolutePredicate.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java @@ -6,13 +6,13 @@ import org.javarosa.xpath.expr.XPathPathExpr; import org.jetbrains.annotations.Nullable; -class CompareChildToAbsolutePredicate { +class CompareChildToAbsoluteExpression { private final XPathPathExpr relativeSide; private final XPathPathExpr absoluteSide; private final XPathExpression original; - public CompareChildToAbsolutePredicate(XPathPathExpr relativeSide, XPathPathExpr absoluteSide, XPathExpression original) { + public CompareChildToAbsoluteExpression(XPathPathExpr relativeSide, XPathPathExpr absoluteSide, XPathExpression original) { this.relativeSide = relativeSide; this.absoluteSide = absoluteSide; this.original = original; @@ -31,7 +31,7 @@ public XPathExpression getOriginal() { } @Nullable - public static CompareChildToAbsolutePredicate parse(XPathExpression expression) { + public static CompareChildToAbsoluteExpression parse(XPathExpression expression) { XPathPathExpr left = null; XPathPathExpr right = null; @@ -50,7 +50,7 @@ public static CompareChildToAbsolutePredicate parse(XPathExpression expression) if (left != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE && right.init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { - return new CompareChildToAbsolutePredicate(left, right, expression); + return new CompareChildToAbsoluteExpression(left, right, expression); } else { return null; } diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 07c6e5937..20959a2bc 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -28,7 +28,7 @@ public class IndexPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - CompareChildToAbsolutePredicate candidate = CompareChildToAbsolutePredicate.parse(predicate); + CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr) { Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), candidate.getRelativeSide().toString()); if (!instanceEqIndexes.containsKey(indexKey)) { @@ -47,7 +47,7 @@ public List filter(DataInstance sourceInstance, TreeReference nod } } - private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbsolutePredicate predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { + private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpression predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); From 68c9667042bdeab6696ebbe64109dfc9123e9dde Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 13:04:21 +0200 Subject: [PATCH 27/47] Add failing tests for supporting more expressions --- .../CompareChildToAbsoluteExpressionTest.java | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 src/test/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionTest.java diff --git a/src/test/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionTest.java b/src/test/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionTest.java new file mode 100644 index 000000000..ce35d1476 --- /dev/null +++ b/src/test/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionTest.java @@ -0,0 +1,72 @@ +package org.javarosa.core.model; + +import org.javarosa.xpath.expr.XPathEqExpr; +import org.javarosa.xpath.expr.XPathNumericLiteral; +import org.javarosa.xpath.expr.XPathPathExpr; +import org.javarosa.xpath.expr.XPathQName; +import org.javarosa.xpath.expr.XPathStep; +import org.javarosa.xpath.expr.XPathStringLiteral; +import org.junit.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; + +public class CompareChildToAbsoluteExpressionTest { + + @Test + public void parse_parsesStringLiteralAsAbsolute() { + XPathEqExpr expression = new XPathEqExpr( + true, + new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{ + new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) } + ), + new XPathStringLiteral("string") + ); + + CompareChildToAbsoluteExpression parsed = CompareChildToAbsoluteExpression.parse(expression); + assertThat(parsed, not(nullValue())); + assertThat(parsed.getRelativeSide(), equalTo(expression.a)); + assertThat(parsed.getAbsoluteSide(), equalTo(expression.b)); + } + + @Test + public void parse_parsesNumericLiteralAsAbsolute() { + XPathEqExpr expression = new XPathEqExpr( + true, + new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{ + new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) } + ), + new XPathNumericLiteral(45.0) + ); + + CompareChildToAbsoluteExpression parsed = CompareChildToAbsoluteExpression.parse(expression); + assertThat(parsed, not(nullValue())); + assertThat(parsed.getRelativeSide(), equalTo(expression.a)); + assertThat(parsed.getAbsoluteSide(), equalTo(expression.b)); + } + + @Test + public void parse_parsesRelativeAndAbsoluteRegardlessOfSide() { + XPathPathExpr relative = new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{ + new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name"))} + ); + XPathPathExpr absolute = new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_ROOT, new XPathStep[]{ + new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("something"))} + ); + + XPathEqExpr ltr = new XPathEqExpr(true, relative, absolute); + XPathEqExpr rtl = new XPathEqExpr(true, absolute, relative); + CompareChildToAbsoluteExpression ltrParsed = CompareChildToAbsoluteExpression.parse(ltr); + CompareChildToAbsoluteExpression rtlParsed = CompareChildToAbsoluteExpression.parse(rtl); + + assertThat(ltrParsed, not(nullValue())); + assertThat(ltrParsed.getRelativeSide(), equalTo(relative)); + assertThat(ltrParsed.getAbsoluteSide(), equalTo(absolute)); + + assertThat(rtlParsed, not(nullValue())); + assertThat(rtlParsed.getRelativeSide(), equalTo(relative)); + assertThat(rtlParsed.getAbsoluteSide(), equalTo(absolute)); + } +} \ No newline at end of file From 140f7cde998c4677f7637ec648dccd800f0cd5a2 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 13:24:11 +0200 Subject: [PATCH 28/47] Support expressions regardless of direction --- .../CompareChildToAbsoluteExpression.java | 55 +++++++++++++------ 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java index da5593f14..5d29d1f61 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java @@ -1,11 +1,16 @@ package org.javarosa.core.model; +import kotlin.Pair; import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; import org.javarosa.xpath.expr.XPathPathExpr; import org.jetbrains.annotations.Nullable; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.Queue; + class CompareChildToAbsoluteExpression { private final XPathPathExpr relativeSide; @@ -32,25 +37,41 @@ public XPathExpression getOriginal() { @Nullable public static CompareChildToAbsoluteExpression parse(XPathExpression expression) { - XPathPathExpr left = null; - XPathPathExpr right = null; - - if (expression instanceof XPathCmpExpr && - ((XPathCmpExpr) expression).a instanceof XPathPathExpr && - ((XPathCmpExpr) expression).b instanceof XPathPathExpr) { - - left = (XPathPathExpr) ((XPathCmpExpr) expression).a; - right = (XPathPathExpr) ((XPathCmpExpr) expression).b; - } else if (expression instanceof XPathEqExpr && - ((XPathEqExpr) expression).a instanceof XPathPathExpr && - ((XPathEqExpr) expression).b instanceof XPathPathExpr) { - left = (XPathPathExpr) ((XPathEqExpr) expression).a; - right = (XPathPathExpr) ((XPathEqExpr) expression).b; + XPathExpression a = null; + XPathExpression b = null; + + if (expression instanceof XPathCmpExpr) { + a = ((XPathCmpExpr) expression).a; + b = ((XPathCmpExpr) expression).b; + } else if (expression instanceof XPathEqExpr) { + a = ((XPathEqExpr) expression).a; + b = ((XPathEqExpr) expression).b; + } + + Pair relativeAndAbsolute = getRelativeAndAbsolute(a, b); + if (relativeAndAbsolute != null) { + return new CompareChildToAbsoluteExpression(relativeAndAbsolute.getFirst(), relativeAndAbsolute.getSecond(), expression); + } else { + return null; + } + } + + private static Pair getRelativeAndAbsolute(XPathExpression a, XPathExpression b) { + XPathPathExpr relative = null; + XPathPathExpr absolute = null; + + Queue subExpressions = new LinkedList<>(Arrays.asList(a, b)); + while (!subExpressions.isEmpty()) { + XPathExpression subExpression = subExpressions.poll(); + if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { + relative = (XPathPathExpr) subExpression; + } else if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { + absolute = (XPathPathExpr) subExpression; + } } - if (left != null && left.init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE && - right.init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { - return new CompareChildToAbsoluteExpression(left, right, expression); + if (relative != null && absolute != null) { + return new Pair<>(relative, absolute); } else { return null; } From 4fe85aef77007edd21262c571312ef5bda0feadc Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 21:45:41 +0200 Subject: [PATCH 29/47] Support numeric and string literals for index caching --- .../core/model/CachingPredicateFilter.java | 20 +++++------- .../CompareChildToAbsoluteExpression.java | 32 +++++++++++++------ .../core/model/IndexPredicateFilter.java | 12 +++---- 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java index 68aecde13..fee28bcf0 100644 --- a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java @@ -28,19 +28,15 @@ public List filter(DataInstance sourceInstance, TreeReference nod CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null) { - Object rightValue = candidate.getAbsoluteSide().eval(sourceInstance, evaluationContext).unpack(); - String key = nodeSet.toString() + predicate + candidate.getRelativeSide() + rightValue.toString(); - - if (key != null) { - if (cachedEvaluations.containsKey(key)) { - return cachedEvaluations.get(key); - } else { - List filtered = next.get(); - cachedEvaluations.put(key, filtered); - return filtered; - } + Object absoluteValue = CompareChildToAbsoluteExpression.evalAbsolute(sourceInstance, evaluationContext, candidate); + String key = nodeSet.toString() + predicate + candidate.getRelativeSide() + absoluteValue.toString(); + + if (cachedEvaluations.containsKey(key)) { + return cachedEvaluations.get(key); } else { - return next.get(); + List filtered = next.get(); + cachedEvaluations.put(key, filtered); + return filtered; } } else { return next.get(); diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java index 5d29d1f61..9716e72fd 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java @@ -1,10 +1,14 @@ package org.javarosa.core.model; import kotlin.Pair; +import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.instance.DataInstance; import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; +import org.javarosa.xpath.expr.XPathNumericLiteral; import org.javarosa.xpath.expr.XPathPathExpr; +import org.javarosa.xpath.expr.XPathStringLiteral; import org.jetbrains.annotations.Nullable; import java.util.Arrays; @@ -14,10 +18,10 @@ class CompareChildToAbsoluteExpression { private final XPathPathExpr relativeSide; - private final XPathPathExpr absoluteSide; + private final XPathExpression absoluteSide; private final XPathExpression original; - public CompareChildToAbsoluteExpression(XPathPathExpr relativeSide, XPathPathExpr absoluteSide, XPathExpression original) { + public CompareChildToAbsoluteExpression(XPathPathExpr relativeSide, XPathExpression absoluteSide, XPathExpression original) { this.relativeSide = relativeSide; this.absoluteSide = absoluteSide; this.original = original; @@ -27,7 +31,7 @@ public XPathPathExpr getRelativeSide() { return relativeSide; } - public XPathPathExpr getAbsoluteSide() { + public XPathExpression getAbsoluteSide() { return absoluteSide; } @@ -48,7 +52,7 @@ public static CompareChildToAbsoluteExpression parse(XPathExpression expression) b = ((XPathEqExpr) expression).b; } - Pair relativeAndAbsolute = getRelativeAndAbsolute(a, b); + Pair relativeAndAbsolute = getRelativeAndAbsolute(a, b); if (relativeAndAbsolute != null) { return new CompareChildToAbsoluteExpression(relativeAndAbsolute.getFirst(), relativeAndAbsolute.getSecond(), expression); } else { @@ -56,17 +60,27 @@ public static CompareChildToAbsoluteExpression parse(XPathExpression expression) } } - private static Pair getRelativeAndAbsolute(XPathExpression a, XPathExpression b) { + public static Object evalAbsolute(DataInstance sourceInstance, EvaluationContext evaluationContext, CompareChildToAbsoluteExpression expression) { + if (expression.absoluteSide instanceof XPathPathExpr) { + return ((XPathPathExpr) expression.getAbsoluteSide()).eval(sourceInstance, evaluationContext).unpack(); + } else { + return expression.absoluteSide.eval(sourceInstance, evaluationContext); + } + } + + private static Pair getRelativeAndAbsolute(XPathExpression a, XPathExpression b) { XPathPathExpr relative = null; - XPathPathExpr absolute = null; + XPathExpression absolute = null; Queue subExpressions = new LinkedList<>(Arrays.asList(a, b)); while (!subExpressions.isEmpty()) { XPathExpression subExpression = subExpressions.poll(); - if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) { + if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE) relative = (XPathPathExpr) subExpression; - } else if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { - absolute = (XPathPathExpr) subExpression; + else if (subExpression instanceof XPathPathExpr && ((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_ROOT) { + absolute = subExpression; + } else if (subExpression instanceof XPathNumericLiteral || subExpression instanceof XPathStringLiteral) { + absolute = subExpression; } } diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 20959a2bc..88f125700 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -30,7 +30,7 @@ public class IndexPredicateFilter implements PredicateFilter { public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr) { - Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), candidate.getRelativeSide().toString()); + Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); if (!instanceEqIndexes.containsKey(indexKey)) { instanceEqIndexes.put(indexKey, new HashMap<>()); } @@ -40,8 +40,8 @@ public List filter(DataInstance sourceInstance, TreeReference nod buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); } - String rightValue = (String) candidate.getAbsoluteSide().eval(sourceInstance, evaluationContext).unpack(); - return index.getOrDefault(rightValue, new ArrayList<>()); + Object absoluteValue = CompareChildToAbsoluteExpression.evalAbsolute(sourceInstance, evaluationContext, candidate); + return index.getOrDefault(absoluteValue.toString(), new ArrayList<>()); } else { return next.get(); } @@ -54,13 +54,13 @@ private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbso EvaluationContext evalContext = evaluationContext.rescope(child, i); Measure.log("IndexEvaluation"); - String leftVal = (String) predicate.getRelativeSide().eval(sourceInstance, evalContext).unpack(); + String relativeValue = (String) predicate.getRelativeSide().eval(sourceInstance, evalContext).unpack(); if (!eqIndex.containsKey(predicate.getRelativeSide().toString())) { - eqIndex.put(leftVal, new ArrayList<>()); + eqIndex.put(relativeValue, new ArrayList<>()); } - eqIndex.get(leftVal).add(child); + eqIndex.get(relativeValue).add(child); } } } From a0c4568f58666d4a85e1f454223814e6b2d4d36d Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Mon, 17 Apr 2023 22:11:56 +0200 Subject: [PATCH 30/47] Don't try and index nested nodesets --- .../javarosa/core/model/IndexPredicateFilter.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 88f125700..9e7afa622 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -29,7 +29,8 @@ public class IndexPredicateFilter implements PredicateFilter { @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); - if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr) { + + if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr && !isNested(nodeSet)) { Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); if (!instanceEqIndexes.containsKey(indexKey)) { instanceEqIndexes.put(indexKey, new HashMap<>()); @@ -47,6 +48,16 @@ public List filter(DataInstance sourceInstance, TreeReference nod } } + private static boolean isNested(TreeReference nodeSet) { + for (int i = 1; i < nodeSet.size(); i++) { + if (nodeSet.getMultiplicity(i) > -1) { + return true; + } + } + + return false; + } + private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpression predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); From e8cdc1bf0a732282114a1a86c58ef05d80842d19 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 19 Apr 2023 15:50:15 +0200 Subject: [PATCH 31/47] Rename class --- ...eFilter.java => CompareChildToAbsoluteExpressionFilter.java} | 2 +- src/main/java/org/javarosa/core/model/TriggerableDag.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename src/main/java/org/javarosa/core/model/{CachingPredicateFilter.java => CompareChildToAbsoluteExpressionFilter.java} (95%) diff --git a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java similarity index 95% rename from src/main/java/org/javarosa/core/model/CachingPredicateFilter.java rename to src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java index fee28bcf0..12f841b79 100644 --- a/src/main/java/org/javarosa/core/model/CachingPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java @@ -18,7 +18,7 @@ * Caches down stream evaluations (in the {@link PredicateFilter} chain) for supported expressions - currently just * {@link XPathCmpExpr} and {@link XPathEqExpr}. Repeated evaluations are fetched in O(1) time. */ -public class CachingPredicateFilter implements PredicateFilter { +public class CompareChildToAbsoluteExpressionFilter implements PredicateFilter { private final Map> cachedEvaluations = new HashMap<>(); diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index c0f9436e9..91850ac58 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -99,7 +99,7 @@ public interface EventNotifierAccessor { private Map relevancePerRepeat = new HashMap<>(); private boolean predicateCaching = true; - private final PredicateFilter cachingPredicateFilter = new CachingPredicateFilter(); + private final PredicateFilter cachingPredicateFilter = new CompareChildToAbsoluteExpressionFilter(); private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(); TriggerableDag(EventNotifierAccessor accessor) { From aa2145286c6ec4478bf8189b9afacc248cbb4d1a Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Thu, 4 May 2023 11:21:40 +0100 Subject: [PATCH 32/47] Move evaluation logic into class --- .../CompareChildToAbsoluteExpression.java | 22 ++++++++++++------- ...ompareChildToAbsoluteExpressionFilter.java | 2 +- .../core/model/IndexPredicateFilter.java | 6 ++--- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java index 9716e72fd..540d8db35 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java @@ -3,6 +3,7 @@ import kotlin.Pair; import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.instance.DataInstance; +import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; @@ -27,6 +28,19 @@ public CompareChildToAbsoluteExpression(XPathPathExpr relativeSide, XPathExpress this.original = original; } + public Object evalRelative(DataInstance sourceInstance, EvaluationContext evaluationContext, TreeReference child, int childIndex) { + EvaluationContext rescopedContext = evaluationContext.rescope(child, childIndex); + return getRelativeSide().eval(sourceInstance, rescopedContext).unpack(); + } + + public Object evalAbsolute(DataInstance sourceInstance, EvaluationContext evaluationContext) { + if (absoluteSide instanceof XPathPathExpr) { + return ((XPathPathExpr) getAbsoluteSide()).eval(sourceInstance, evaluationContext).unpack(); + } else { + return absoluteSide.eval(sourceInstance, evaluationContext); + } + } + public XPathPathExpr getRelativeSide() { return relativeSide; } @@ -60,14 +74,6 @@ public static CompareChildToAbsoluteExpression parse(XPathExpression expression) } } - public static Object evalAbsolute(DataInstance sourceInstance, EvaluationContext evaluationContext, CompareChildToAbsoluteExpression expression) { - if (expression.absoluteSide instanceof XPathPathExpr) { - return ((XPathPathExpr) expression.getAbsoluteSide()).eval(sourceInstance, evaluationContext).unpack(); - } else { - return expression.absoluteSide.eval(sourceInstance, evaluationContext); - } - } - private static Pair getRelativeAndAbsolute(XPathExpression a, XPathExpression b) { XPathPathExpr relative = null; XPathExpression absolute = null; diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java index 12f841b79..c3de128e9 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java @@ -28,7 +28,7 @@ public List filter(DataInstance sourceInstance, TreeReference nod CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null) { - Object absoluteValue = CompareChildToAbsoluteExpression.evalAbsolute(sourceInstance, evaluationContext, candidate); + Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); String key = nodeSet.toString() + predicate + candidate.getRelativeSide() + absoluteValue.toString(); if (cachedEvaluations.containsKey(key)) { diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 9e7afa622..48360cd6d 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -41,7 +41,7 @@ public List filter(DataInstance sourceInstance, TreeReference nod buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); } - Object absoluteValue = CompareChildToAbsoluteExpression.evalAbsolute(sourceInstance, evaluationContext, candidate); + Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); return index.getOrDefault(absoluteValue.toString(), new ArrayList<>()); } else { return next.get(); @@ -62,10 +62,8 @@ private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbso for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); - EvaluationContext evalContext = evaluationContext.rescope(child, i); - Measure.log("IndexEvaluation"); - String relativeValue = (String) predicate.getRelativeSide().eval(sourceInstance, evalContext).unpack(); + String relativeValue = predicate.evalRelative(sourceInstance, evaluationContext, child, i).toString(); if (!eqIndex.containsKey(predicate.getRelativeSide().toString())) { eqIndex.put(relativeValue, new ArrayList<>()); From 59cb3f9293298c9c1874f0e7fc90f7107708a0cb Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 5 May 2023 17:26:48 +0300 Subject: [PATCH 33/47] Fix different types of eq being confused --- .../core/model/IndexPredicateFilter.java | 25 +++++++++------ .../org/javarosa/xpath/expr/XPathEqExpr.java | 4 +++ .../core/model/test/PredicateCachingTest.java | 31 +++++++++++++++++++ 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 48360cd6d..344a7fb74 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -31,18 +31,23 @@ public List filter(DataInstance sourceInstance, TreeReference nod CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr && !isNested(nodeSet)) { - Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); - if (!instanceEqIndexes.containsKey(indexKey)) { - instanceEqIndexes.put(indexKey, new HashMap<>()); - } + XPathEqExpr original = (XPathEqExpr) candidate.getOriginal(); + if (original.isEqual()) { + Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); + if (!instanceEqIndexes.containsKey(indexKey)) { + instanceEqIndexes.put(indexKey, new HashMap<>()); + } - Map> index = instanceEqIndexes.get(indexKey); - if (index.isEmpty()) { - buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); - } + Map> index = instanceEqIndexes.get(indexKey); + if (index.isEmpty()) { + buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); + } - Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); - return index.getOrDefault(absoluteValue.toString(), new ArrayList<>()); + Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); + return index.getOrDefault(absoluteValue.toString(), new ArrayList<>()); + } else { + return next.get(); + } } else { return next.get(); } diff --git a/src/main/java/org/javarosa/xpath/expr/XPathEqExpr.java b/src/main/java/org/javarosa/xpath/expr/XPathEqExpr.java index c52a1f336..16c45c60f 100644 --- a/src/main/java/org/javarosa/xpath/expr/XPathEqExpr.java +++ b/src/main/java/org/javarosa/xpath/expr/XPathEqExpr.java @@ -100,6 +100,10 @@ public void writeExternal(DataOutputStream out) throws IOException { super.writeExternal(out); } + public boolean isEqual() { + return equal; + } + private String id() { return "@" + Integer.toHexString(System.identityHashCode(this)); } diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 944d0b5c6..d6787231e 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -437,4 +437,35 @@ public void differentEqExpressionsAreNotConfused() throws Exception { assertThat(scenario.answerOf("/data/calc1").getValue(), equalTo("A")); assertThat(scenario.answerOf("/data/calc2").getValue(), equalTo("B")); } + + @Test + public void differentKindsOfEqExpressionsAreNotConfused() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calc1"), + t("calc2"), + t("input") + )), + instance("instance", + item("a", "A"), + item("b", "B") + ), + bind("/data/calc1").type("string") + .calculate("instance('instance')/root/item[value = 'a']/label"), + bind("/data/calc2").type("string") + .calculate("instance('instance')/root/item[value != 'a']/label"), + bind("/data/input").type("string") + ) + ), + body( + input("/data/input") + ) + )); + + assertThat(scenario.answerOf("/data/calc1").getValue(), equalTo("A")); + assertThat(scenario.answerOf("/data/calc2").getValue(), equalTo("B")); + } } From 2bb898f8d7a9664dd7a56d07de6c5e873d713c3e Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 5 May 2023 17:47:14 +0300 Subject: [PATCH 34/47] Prefer path expressions in regression tests where it's possible --- .../core/model/test/PredicateCachingTest.java | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index d6787231e..40ca105a3 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -251,15 +251,17 @@ public void predicatesOnDifferentChildNamesDoNotGetConfused() throws Exception { ) ), bind("/data/cat").type("string") - .calculate("instance('instance')/root/cat[name = 'Vinnie']/age"), + .calculate("instance('instance')/root/cat[name = /data/input]/age"), bind("/data/dog").type("string") - .calculate("instance('instance')/root/dog[name = 'Vinnie']/age"), + .calculate("instance('instance')/root/dog[name = /data/input]/age"), bind("/data/input").type("string") ) ), body(input("/data/input")) )); + scenario.answer("/data/input", "Vinnie"); + assertThat(scenario.answerOf("/data/cat").getValue(), equalTo("12")); assertThat(scenario.answerOf("/data/dog").getValue(), equalTo("9")); } @@ -329,7 +331,7 @@ public void eqExpressionsWorkIfBothSidesAreRelative() throws Exception { } @Test - public void predicatesInMultipleSetsDoNotGetConfused() throws Exception { + public void nestedPredicatesDoNotGetConfused() throws Exception { Scenario scenario = Scenario.init("Some form", html( head( title("Some form"), @@ -337,7 +339,8 @@ public void predicatesInMultipleSetsDoNotGetConfused() throws Exception { mainInstance(t("data id=\"some-form\"", t("calc"), t("calc2"), - t("input") + t("input1"), + t("input2") )), instance("instance", t("item", @@ -357,15 +360,22 @@ public void predicatesInMultipleSetsDoNotGetConfused() throws Exception { ) ), bind("/data/calc").type("string") - .calculate("instance('instance')/root/item[value = 'A'][count = '3']/id"), + .calculate("instance('instance')/root/item[value = /data/input1][count = '3']/id"), bind("/data/calc2").type("string") - .calculate("instance('instance')/root/item[value = 'B'][count = '3']/id"), - bind("/data/input").type("string") + .calculate("instance('instance')/root/item[value = /data/input2][count = '3']/id"), + bind("/data/input1").type("string"), + bind("/data/input2").type("string") ) ), - body(input("/data/input")) + body( + input("/data/input1"), + input("/data/input2") + ) )); + scenario.answer("/data/input1", "A"); + scenario.answer("/data/input2", "B"); + assertThat(scenario.answerOf("/data/calc").getValue(), equalTo("A3")); assertThat(scenario.answerOf("/data/calc2"), equalTo(null)); } From fa1a8240490b38a80dcad2d0603fb72d8eeaf12f Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 5 May 2023 17:52:47 +0300 Subject: [PATCH 35/47] Correct name of test --- .../java/org/javarosa/core/model/test/PredicateCachingTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index 40ca105a3..a0460255e 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -164,7 +164,7 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { } @Test - public void repeatedCompPredicatesAreOnlyEvaluatedOnce() throws Exception { + public void repeatedCompPredicatesWithSameAnswerAreOnlyEvaluatedOnce() throws Exception { Scenario scenario = Scenario.init("Some form", html( head( title("Some form"), From 2753251430dab7e5bf07d8a3d9e4ca8e906e07a3 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 9 May 2023 14:03:26 +0300 Subject: [PATCH 36/47] Don't apply caching to expressions on main instance --- ...ompareChildToAbsoluteExpressionFilter.java | 5 ++- .../core/model/IndexPredicateFilter.java | 7 +++- .../core/model/test/PredicateCachingTest.java | 38 +++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java index c3de128e9..61b7314a7 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java @@ -25,8 +25,11 @@ public class CompareChildToAbsoluteExpressionFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); + if (sourceInstance.getInstanceId() == null) { + return next.get(); + } + CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); if (candidate != null) { Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); String key = nodeSet.toString() + predicate + candidate.getRelativeSide() + absoluteValue.toString(); diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 344a7fb74..db67c1ba5 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -28,9 +28,12 @@ public class IndexPredicateFilter implements PredicateFilter { @Nullable @Override public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { - CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); + if (sourceInstance.getInstanceId() == null || isNested(nodeSet) || !(predicate instanceof XPathEqExpr)) { + return next.get(); + } - if (candidate != null && candidate.getOriginal() instanceof XPathEqExpr && !isNested(nodeSet)) { + CompareChildToAbsoluteExpression candidate = CompareChildToAbsoluteExpression.parse(predicate); + if (candidate != null) { XPathEqExpr original = (XPathEqExpr) candidate.getOriginal(); if (original.isEqual()) { Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index a0460255e..d3349a16b 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -10,6 +10,7 @@ import static org.hamcrest.Matchers.lessThan; import static org.javarosa.core.util.BindBuilderXFormsElement.bind; import static org.javarosa.core.util.XFormsElement.body; +import static org.javarosa.core.util.XFormsElement.group; import static org.javarosa.core.util.XFormsElement.head; import static org.javarosa.core.util.XFormsElement.html; import static org.javarosa.core.util.XFormsElement.input; @@ -17,6 +18,7 @@ import static org.javarosa.core.util.XFormsElement.item; import static org.javarosa.core.util.XFormsElement.mainInstance; import static org.javarosa.core.util.XFormsElement.model; +import static org.javarosa.core.util.XFormsElement.repeat; import static org.javarosa.core.util.XFormsElement.t; import static org.javarosa.core.util.XFormsElement.title; @@ -478,4 +480,40 @@ public void differentKindsOfEqExpressionsAreNotConfused() throws Exception { assertThat(scenario.answerOf("/data/calc1").getValue(), equalTo("A")); assertThat(scenario.answerOf("/data/calc2").getValue(), equalTo("B")); } + + @Test + public void repeatsUsedInCalculatesStayUpToDate() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("repeat", + t("name"), + t("age") + ), + t("result") + )), + bind("/data/repeat/input").type("string"), + bind("/data/result").type("string").calculate("/data/repeat[name = 'John Bell']/age") + ) + ), + body( + group("/data/repeat", + repeat("/data/repeat", + input("/data/repeat/name"), + input("/data/repeat/age") + ) + ) + ) + )); + + assertThat(scenario.answerOf("/data/result"), equalTo(null)); + + scenario.createNewRepeat("/data/repeat"); + scenario.answer("/data/repeat[0]/name", "John Bell"); + scenario.answer("/data/repeat[0]/age", "70"); + + assertThat(scenario.answerOf("/data/result").getValue(), equalTo("70")); + } } From 7ded84f36a1a50e80fc8478aabb516313748eafd Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 9 May 2023 14:23:44 +0300 Subject: [PATCH 37/47] Add caching for first predicate in chain --- .../core/model/IndexPredicateFilter.java | 2 +- .../model/condition/EvaluationContext.java | 16 +++--- .../core/model/test/PredicateCachingTest.java | 52 +++++++++++++++++++ 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index db67c1ba5..454b59508 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -73,7 +73,7 @@ private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbso Measure.log("IndexEvaluation"); String relativeValue = predicate.evalRelative(sourceInstance, evaluationContext, child, i).toString(); - if (!eqIndex.containsKey(predicate.getRelativeSide().toString())) { + if (!eqIndex.containsKey(relativeValue)) { eqIndex.put(relativeValue, new ArrayList<>()); } diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index eb4a4f018..d49bae421 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -330,18 +330,18 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so TreeReference nodeSetRef = workingRef.clone(); nodeSetRef.add(name, -1); - List filterChain; - if (predicates.size() == 1) { - filterChain = predicateFilterChain; - } else { - filterChain = singletonList(new XPathEvalPredicateFilter()); - } + for (int i = 0; i < predicates.size(); i++) { + List filterChain; + if (i == 0) { + filterChain = predicateFilterChain; + } else { + filterChain = singletonList(new XPathEvalPredicateFilter()); + } - for (XPathExpression xpe : predicates) { List passed = filterWithPredicate( sourceInstance, nodeSetRef, - xpe, + predicates.get(i), treeReferences, filterChain ); diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index d3349a16b..e69f1c6df 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -165,6 +165,58 @@ public void repeatedEqPredicatesAreOnlyEvaluatedOnce() throws Exception { assertThat(evaluations, lessThan(4)); } + @Test + public void firstPredicateInMultipleEqPredicatesAreOnlyEvaluatedOnce() throws Exception { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("calc"), + t("input1"), + t("input2") + )), + instance("instance", + t("item", + t("value", "A"), + t("count", "2"), + t("id", "A2") + ), + t("item", + t("value", "A"), + t("count", "3"), + t("id", "A3") + ), + t("item", + t("value", "B"), + t("count", "2"), + t("id", "B2") + ) + ), + bind("/data/calc").type("string") + .calculate("instance('instance')/root/item[value = /data/input1][count = /data/input2]/id"), + bind("/data/input1").type("string"), + bind("/data/input2").type("string") + ) + ), + body( + input("/data/input1"), + input("/data/input2") + ) + )); + + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { + scenario.answer("/data/input1", "A"); + scenario.answer("/data/input2", "3"); + + scenario.answer("/data/input1", "A"); + scenario.answer("/data/input2", "2"); + }); + + // Check that we do less than size of (secondary instance + filtered secondary instance) * number of times we answer + assertThat(evaluations, lessThan(20)); + } + @Test public void repeatedCompPredicatesWithSameAnswerAreOnlyEvaluatedOnce() throws Exception { Scenario scenario = Scenario.init("Some form", html( From b496772e8ea1fa65e398fb86c3ae579f7bc32f21 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Tue, 9 May 2023 14:50:20 +0300 Subject: [PATCH 38/47] Pull indexing data structure code out of predicate filter --- .../core/model/InMemTreeReferenceIndex.java | 44 +++++++++++++++++++ .../core/model/IndexPredicateFilter.java | 33 +++++--------- .../javarosa/core/model/TriggerableDag.java | 2 +- .../model/instance/TreeReferenceIndex.java | 11 +++++ 4 files changed, 68 insertions(+), 22 deletions(-) create mode 100644 src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java create mode 100644 src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java diff --git a/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java b/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java new file mode 100644 index 000000000..216679b2e --- /dev/null +++ b/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java @@ -0,0 +1,44 @@ +package org.javarosa.core.model; + +import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.core.model.instance.TreeReferenceIndex; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static java.util.Collections.emptyList; + +class InMemTreeReferenceIndex implements TreeReferenceIndex { + + private final Map>> map = new HashMap<>(); + + @Override + public boolean contains(String section) { + return map.containsKey(section); + } + + @Override + public void add(String section, String key, TreeReference reference) { + if (!map.containsKey(section)) { + map.put(section, new HashMap<>()); + } + + Map> sectionMap = map.get(section); + if (!sectionMap.containsKey(key)) { + sectionMap.put(key, new ArrayList<>()); + } + + sectionMap.get(key).add(reference); + } + + @Override + public List lookup(String section, String key) { + if (map.containsKey(section) && map.get(section).containsKey(key)) { + return map.get(section).get(key); + } else { + return emptyList(); + } + } +} diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 454b59508..8eafc65bd 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -1,19 +1,16 @@ package org.javarosa.core.model; -import kotlin.Pair; import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; +import org.javarosa.core.model.instance.TreeReferenceIndex; import org.javarosa.measure.Measure; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; import org.jetbrains.annotations.Nullable; -import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.function.Supplier; /** @@ -23,7 +20,11 @@ */ public class IndexPredicateFilter implements PredicateFilter { - private final Map, Map>> instanceEqIndexes = new HashMap<>(); + private final TreeReferenceIndex index; + + public IndexPredicateFilter(TreeReferenceIndex treeReferenceIndex) { + this.index = treeReferenceIndex; + } @Nullable @Override @@ -36,18 +37,13 @@ public List filter(DataInstance sourceInstance, TreeReference nod if (candidate != null) { XPathEqExpr original = (XPathEqExpr) candidate.getOriginal(); if (original.isEqual()) { - Pair indexKey = new Pair<>(sourceInstance.getInstanceId(), nodeSet.toString() + candidate.getRelativeSide().toString()); - if (!instanceEqIndexes.containsKey(indexKey)) { - instanceEqIndexes.put(indexKey, new HashMap<>()); - } - - Map> index = instanceEqIndexes.get(indexKey); - if (index.isEmpty()) { - buildEqIndex(sourceInstance, candidate, children, evaluationContext, index); + String section = sourceInstance.getInstanceId() + nodeSet + candidate.getRelativeSide().toString(); + if (!index.contains(section)) { + buildIndex(sourceInstance, candidate, children, evaluationContext, section); } Object absoluteValue = candidate.evalAbsolute(sourceInstance, evaluationContext); - return index.getOrDefault(absoluteValue.toString(), new ArrayList<>()); + return index.lookup(section, absoluteValue.toString()); } else { return next.get(); } @@ -66,18 +62,13 @@ private static boolean isNested(TreeReference nodeSet) { return false; } - private static void buildEqIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpression predicate, List children, EvaluationContext evaluationContext, Map> eqIndex) { + private void buildIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpression predicate, List children, EvaluationContext evaluationContext, String section) { for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); Measure.log("IndexEvaluation"); String relativeValue = predicate.evalRelative(sourceInstance, evaluationContext, child, i).toString(); - - if (!eqIndex.containsKey(relativeValue)) { - eqIndex.put(relativeValue, new ArrayList<>()); - } - - eqIndex.get(relativeValue).add(child); + index.add(section, relativeValue, child); } } } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 91850ac58..157ac101f 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -100,7 +100,7 @@ public interface EventNotifierAccessor { private boolean predicateCaching = true; private final PredicateFilter cachingPredicateFilter = new CompareChildToAbsoluteExpressionFilter(); - private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(); + private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(new InMemTreeReferenceIndex()); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; diff --git a/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java b/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java new file mode 100644 index 000000000..4268bede4 --- /dev/null +++ b/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java @@ -0,0 +1,11 @@ +package org.javarosa.core.model.instance; + +import java.util.List; + +public interface TreeReferenceIndex { + boolean contains(String section); + + void add(String section, String key, TreeReference reference); + + List lookup(String section, String key); +} From 3cc5c7639543252b1cc5faef5cb72c60a3962709 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 10 May 2023 12:20:55 +0300 Subject: [PATCH 39/47] Correct Measure implementation --- src/main/java/org/javarosa/measure/Measure.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/measure/Measure.java b/src/main/java/org/javarosa/measure/Measure.java index 1006d9055..1d8170d33 100644 --- a/src/main/java/org/javarosa/measure/Measure.java +++ b/src/main/java/org/javarosa/measure/Measure.java @@ -17,7 +17,7 @@ public static int withMeasure(String event, Runnable work) { start(); work.run(); int count = getCount(event); - start(); + stop(); return count; } @@ -31,7 +31,7 @@ public static int withMeasure(List events, Runnable work) { count += getCount(event); } - start(); + stop(); return count; } From adc1463642442d0052afd915d2fa9d802d9d116d Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Wed, 10 May 2023 13:47:47 +0300 Subject: [PATCH 40/47] Remove unused method --- src/main/java/org/javarosa/measure/Measure.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/main/java/org/javarosa/measure/Measure.java b/src/main/java/org/javarosa/measure/Measure.java index 1d8170d33..3b6ff87c4 100644 --- a/src/main/java/org/javarosa/measure/Measure.java +++ b/src/main/java/org/javarosa/measure/Measure.java @@ -13,15 +13,6 @@ private Measure() { } - public static int withMeasure(String event, Runnable work) { - start(); - work.run(); - int count = getCount(event); - stop(); - - return count; - } - public static int withMeasure(List events, Runnable work) { start(); work.run(); From 31fbd2f954dc278a7bc4d170c175fc603af1abba Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 10:19:04 +0300 Subject: [PATCH 41/47] Nest interface --- .../javarosa/core/model/InMemTreeReferenceIndex.java | 3 +-- .../org/javarosa/core/model/IndexPredicateFilter.java | 9 ++++++++- .../core/model/instance/TreeReferenceIndex.java | 11 ----------- 3 files changed, 9 insertions(+), 14 deletions(-) delete mode 100644 src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java diff --git a/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java b/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java index 216679b2e..e8630fd4d 100644 --- a/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java +++ b/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java @@ -1,7 +1,6 @@ package org.javarosa.core.model; import org.javarosa.core.model.instance.TreeReference; -import org.javarosa.core.model.instance.TreeReferenceIndex; import java.util.ArrayList; import java.util.HashMap; @@ -10,7 +9,7 @@ import static java.util.Collections.emptyList; -class InMemTreeReferenceIndex implements TreeReferenceIndex { +class InMemTreeReferenceIndex implements IndexPredicateFilter.TreeReferenceIndex { private final Map>> map = new HashMap<>(); diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 8eafc65bd..f6f0a0c67 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -4,7 +4,6 @@ import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; -import org.javarosa.core.model.instance.TreeReferenceIndex; import org.javarosa.measure.Measure; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; @@ -71,4 +70,12 @@ private void buildIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpre index.add(section, relativeValue, child); } } + + interface TreeReferenceIndex { + boolean contains(String section); + + void add(String section, String key, TreeReference reference); + + List lookup(String section, String key); + } } diff --git a/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java b/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java deleted file mode 100644 index 4268bede4..000000000 --- a/src/main/java/org/javarosa/core/model/instance/TreeReferenceIndex.java +++ /dev/null @@ -1,11 +0,0 @@ -package org.javarosa.core.model.instance; - -import java.util.List; - -public interface TreeReferenceIndex { - boolean contains(String section); - - void add(String section, String key, TreeReference reference); - - List lookup(String section, String key); -} From fcf57035c988ad9ee0d8ec4395b3d59d2872cd47 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 10:39:04 +0300 Subject: [PATCH 42/47] Create default filter chain --- .../org/javarosa/core/model/condition/EvaluationContext.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index d49bae421..c0e4be3bc 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -66,7 +66,8 @@ public class EvaluationContext { private DataInstance instance; private int[] predicateEvaluationProgress; - private List predicateFilterChain = singletonList(new XPathEvalPredicateFilter()); + private static final List DEFAULT_PREDICATE_FILTER_CHAIN = singletonList(new XPathEvalPredicateFilter()); + private List predicateFilterChain = DEFAULT_PREDICATE_FILTER_CHAIN; /** * Copy Constructor @@ -335,7 +336,7 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so if (i == 0) { filterChain = predicateFilterChain; } else { - filterChain = singletonList(new XPathEvalPredicateFilter()); + filterChain = DEFAULT_PREDICATE_FILTER_CHAIN; } List passed = filterWithPredicate( From a30fade23aeba63aae991cd4b08a78741259ebb0 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 10:43:04 +0300 Subject: [PATCH 43/47] Remove interface that's only used in one class --- .../core/model/InMemTreeReferenceIndex.java | 43 ------------------- .../core/model/IndexPredicateFilter.java | 41 ++++++++++++++---- .../javarosa/core/model/TriggerableDag.java | 2 +- 3 files changed, 33 insertions(+), 53 deletions(-) delete mode 100644 src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java diff --git a/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java b/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java deleted file mode 100644 index e8630fd4d..000000000 --- a/src/main/java/org/javarosa/core/model/InMemTreeReferenceIndex.java +++ /dev/null @@ -1,43 +0,0 @@ -package org.javarosa.core.model; - -import org.javarosa.core.model.instance.TreeReference; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import static java.util.Collections.emptyList; - -class InMemTreeReferenceIndex implements IndexPredicateFilter.TreeReferenceIndex { - - private final Map>> map = new HashMap<>(); - - @Override - public boolean contains(String section) { - return map.containsKey(section); - } - - @Override - public void add(String section, String key, TreeReference reference) { - if (!map.containsKey(section)) { - map.put(section, new HashMap<>()); - } - - Map> sectionMap = map.get(section); - if (!sectionMap.containsKey(key)) { - sectionMap.put(key, new ArrayList<>()); - } - - sectionMap.get(key).add(reference); - } - - @Override - public List lookup(String section, String key) { - if (map.containsKey(section) && map.get(section).containsKey(key)) { - return map.get(section).get(key); - } else { - return emptyList(); - } - } -} diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index f6f0a0c67..964147e51 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -9,9 +9,14 @@ import org.javarosa.xpath.expr.XPathExpression; import org.jetbrains.annotations.Nullable; +import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.function.Supplier; +import static java.util.Collections.emptyList; + /** * Uses a (lazily constructed) index to evaluate a predicate for supported expressions - currently just * {@link XPathEqExpr} where one side is relative to the instance child being filtered. Evaluations are fetched in @@ -19,11 +24,7 @@ */ public class IndexPredicateFilter implements PredicateFilter { - private final TreeReferenceIndex index; - - public IndexPredicateFilter(TreeReferenceIndex treeReferenceIndex) { - this.index = treeReferenceIndex; - } + private final InMemTreeReferenceIndex index = new InMemTreeReferenceIndex(); @Nullable @Override @@ -71,11 +72,33 @@ private void buildIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpre } } - interface TreeReferenceIndex { - boolean contains(String section); + private static class InMemTreeReferenceIndex { + + private final Map>> map = new HashMap<>(); + + public boolean contains(String section) { + return map.containsKey(section); + } + + public void add(String section, String key, TreeReference reference) { + if (!map.containsKey(section)) { + map.put(section, new HashMap<>()); + } + + Map> sectionMap = map.get(section); + if (!sectionMap.containsKey(key)) { + sectionMap.put(key, new ArrayList<>()); + } - void add(String section, String key, TreeReference reference); + sectionMap.get(key).add(reference); + } - List lookup(String section, String key); + public List lookup(String section, String key) { + if (map.containsKey(section) && map.get(section).containsKey(key)) { + return map.get(section).get(key); + } else { + return emptyList(); + } + } } } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 157ac101f..91850ac58 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -100,7 +100,7 @@ public interface EventNotifierAccessor { private boolean predicateCaching = true; private final PredicateFilter cachingPredicateFilter = new CompareChildToAbsoluteExpressionFilter(); - private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(new InMemTreeReferenceIndex()); + private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; From 44d944adddf44195caa38ea30925193f3aafe89d Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 10:55:55 +0300 Subject: [PATCH 44/47] Add ability to add predicate filter --- .../java/org/javarosa/core/model/FormDef.java | 5 +++++ .../javarosa/core/model/TriggerableDag.java | 20 +++++++++++++------ .../form/api/FormEntryController.java | 5 +++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/FormDef.java b/src/main/java/org/javarosa/core/model/FormDef.java index ba257c9ed..343ba8310 100644 --- a/src/main/java/org/javarosa/core/model/FormDef.java +++ b/src/main/java/org/javarosa/core/model/FormDef.java @@ -24,6 +24,7 @@ import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.IConditionExpr; import org.javarosa.core.model.condition.IFunctionHandler; +import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.condition.Triggerable; import org.javarosa.core.model.data.IAnswerData; import org.javarosa.core.model.data.MultipleItemsData; @@ -1707,4 +1708,8 @@ public Extras getExtras() { public void disablePredicateCaching() { dagImpl.disablePredicateCaching(); } + + public void addPredicateFilter(PredicateFilter predicateFilter) { + dagImpl.addPredicateFilter(predicateFilter); + } } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 91850ac58..270f6fd12 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -40,7 +40,6 @@ import java.io.DataOutputStream; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -48,7 +47,10 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Queue; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import static java.util.Collections.emptySet; @@ -101,6 +103,7 @@ public interface EventNotifierAccessor { private boolean predicateCaching = true; private final PredicateFilter cachingPredicateFilter = new CompareChildToAbsoluteExpressionFilter(); private final PredicateFilter indexPredicateFilter = new IndexPredicateFilter(); + private final Queue customPredicateFilters = new LinkedList<>(); TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; @@ -523,11 +526,12 @@ private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext context; if (predicateCaching) { - context = new EvaluationContext(evalContext, Arrays.asList( - indexPredicateFilter, - cachingPredicateFilter, - new IdempotentPredicateCache() - )); + List filters = Stream.concat( + customPredicateFilters.stream(), + Stream.of(indexPredicateFilter, cachingPredicateFilter, new IdempotentPredicateCache()) + ).collect(Collectors.toList()); + + context = new EvaluationContext(evalContext, filters); } else { context = evalContext; } @@ -746,4 +750,8 @@ private List getRecalculates() { public void disablePredicateCaching() { this.predicateCaching = false; } + + public void addPredicateFilter(PredicateFilter predicateFilter) { + customPredicateFilters.add(predicateFilter); + } } diff --git a/src/main/java/org/javarosa/form/api/FormEntryController.java b/src/main/java/org/javarosa/form/api/FormEntryController.java index a3d788c01..c66c0dda4 100644 --- a/src/main/java/org/javarosa/form/api/FormEntryController.java +++ b/src/main/java/org/javarosa/form/api/FormEntryController.java @@ -21,6 +21,7 @@ import org.javarosa.core.model.GroupDef; import org.javarosa.core.model.IFormElement; import org.javarosa.core.model.QuestionDef; +import org.javarosa.core.model.condition.PredicateFilter; import org.javarosa.core.model.data.IAnswerData; import org.javarosa.core.model.instance.InvalidReferenceException; import org.javarosa.core.model.instance.TreeElement; @@ -349,4 +350,8 @@ private static FormIndex getRepeatGroupIndex(FormIndex index, FormDef formDef) { public void disablePredicateCaching() { model.getForm().disablePredicateCaching(); } + + public void addPredicateFilter(PredicateFilter predicateFilter) { + model.getForm().addPredicateFilter(predicateFilter); + } } From 93a6d6cb9f94ffb0c3ff18bf710ca1044f5e0a7b Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 11:16:54 +0300 Subject: [PATCH 45/47] Improve interface for clients --- .../model/CompareChildToAbsoluteExpression.java | 2 +- .../CompareChildToAbsoluteExpressionFilter.java | 6 +++--- .../core/model/IdempotentPredicateCache.java | 6 +++--- .../core/model/IndexPredicateFilter.java | 6 +++--- .../core/model/condition/EvaluationContext.java | 3 +++ .../core/model/condition/PredicateFilter.java | 16 ++++++++-------- .../condition/XPathEvalPredicateFilter.java | 12 ++++++------ 7 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java index 540d8db35..b1cde3afe 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java @@ -16,7 +16,7 @@ import java.util.LinkedList; import java.util.Queue; -class CompareChildToAbsoluteExpression { +public class CompareChildToAbsoluteExpression { private final XPathPathExpr relativeSide; private final XPathExpression absoluteSide; diff --git a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java index 61b7314a7..733f3fc88 100644 --- a/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java +++ b/src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionFilter.java @@ -7,7 +7,7 @@ import org.javarosa.xpath.expr.XPathCmpExpr; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.NotNull; import java.util.HashMap; import java.util.List; @@ -22,9 +22,9 @@ public class CompareChildToAbsoluteExpressionFilter implements PredicateFilter { private final Map> cachedEvaluations = new HashMap<>(); - @Nullable + @NotNull @Override - public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + public List filter(@NotNull DataInstance sourceInstance, @NotNull TreeReference nodeSet, @NotNull XPathExpression predicate, @NotNull List children, @NotNull EvaluationContext evaluationContext, @NotNull Supplier> next) { if (sourceInstance.getInstanceId() == null) { return next.get(); } diff --git a/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java b/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java index ff01a98fa..06042657e 100644 --- a/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java +++ b/src/main/java/org/javarosa/core/model/IdempotentPredicateCache.java @@ -5,7 +5,7 @@ import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.NotNull; import java.util.HashMap; import java.util.List; @@ -21,9 +21,9 @@ public class IdempotentPredicateCache implements PredicateFilter { private final Map> cachedEvaluations = new HashMap<>(); - @Nullable + @NotNull @Override - public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + public List filter(@NotNull DataInstance sourceInstance, @NotNull TreeReference nodeSet, @NotNull XPathExpression predicate, @NotNull List children, @NotNull EvaluationContext evaluationContext, @NotNull Supplier> next) { String key = getKey(nodeSet, predicate); if (cachedEvaluations.containsKey(key)) { diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 964147e51..6a8ddb588 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -7,7 +7,7 @@ import org.javarosa.measure.Measure; import org.javarosa.xpath.expr.XPathEqExpr; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.NotNull; import java.util.ArrayList; import java.util.HashMap; @@ -26,9 +26,9 @@ public class IndexPredicateFilter implements PredicateFilter { private final InMemTreeReferenceIndex index = new InMemTreeReferenceIndex(); - @Nullable + @NotNull @Override - public List filter(DataInstance sourceInstance, TreeReference nodeSet, XPathExpression predicate, List children, EvaluationContext evaluationContext, Supplier> next) { + public List filter(@NotNull DataInstance sourceInstance, @NotNull TreeReference nodeSet, @NotNull XPathExpression predicate, @NotNull List children, @NotNull EvaluationContext evaluationContext, @NotNull Supplier> next) { if (sourceInstance.getInstanceId() == null || isNested(nodeSet) || !(predicate instanceof XPathEqExpr)) { return next.get(); } diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index c0e4be3bc..6965e2d62 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -23,6 +23,7 @@ import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.IExprDataType; import org.javarosa.xpath.expr.XPathExpression; +import org.jetbrains.annotations.NotNull; import java.util.ArrayList; import java.util.Date; @@ -361,10 +362,12 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } } + @NotNull private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, List filterChain) { return filterWithPredicate(sourceInstance, treeReference, predicate, children, 0, filterChain); } + @NotNull private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, int i, List filterChain) { return filterChain.get(i).filter(sourceInstance, treeReference, predicate, children, this, () -> { return filterWithPredicate(sourceInstance, treeReference, predicate, children, i + 1, filterChain); diff --git a/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java index 4ee0b073d..ec754fa3f 100644 --- a/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/PredicateFilter.java @@ -3,18 +3,18 @@ import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xpath.expr.XPathExpression; -import org.jetbrains.annotations.Nullable; +import org.jetbrains.annotations.NotNull; import java.util.List; import java.util.function.Supplier; public interface PredicateFilter { - @Nullable - List filter(DataInstance sourceInstance, - TreeReference nodeSet, - XPathExpression predicate, - List children, - EvaluationContext evaluationContext, - Supplier> next); + @NotNull + List filter(@NotNull DataInstance sourceInstance, + @NotNull TreeReference nodeSet, + @NotNull XPathExpression predicate, + @NotNull List children, + @NotNull EvaluationContext evaluationContext, + @NotNull Supplier> next); } diff --git a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java index 1f23f3880..092d2acb5 100644 --- a/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/condition/XPathEvalPredicateFilter.java @@ -14,12 +14,12 @@ class XPathEvalPredicateFilter implements PredicateFilter { @NotNull @Override - public List filter(DataInstance sourceInstance, - TreeReference nodeSet, - XPathExpression predicate, - List children, - EvaluationContext evaluationContext, - Supplier> nextFilter) { + public List filter(@NotNull DataInstance sourceInstance, + @NotNull TreeReference nodeSet, + @NotNull XPathExpression predicate, + @NotNull List children, + @NotNull EvaluationContext evaluationContext, + @NotNull Supplier> nextFilter) { List predicatePassed = new ArrayList<>(children.size()); for (int i = 0; i < children.size(); ++i) { //if there are predicates then we need to see if e.nextElement meets the standard of the predicate From 2dc034da2ba37c9f55305f97857f6a30402f8538 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 11:23:02 +0300 Subject: [PATCH 46/47] Move nesting check to EvaluationContext --- .../javarosa/core/model/IndexPredicateFilter.java | 12 +----------- .../core/model/condition/EvaluationContext.java | 12 +++++++++++- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java index 6a8ddb588..c46fea708 100644 --- a/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java +++ b/src/main/java/org/javarosa/core/model/IndexPredicateFilter.java @@ -29,7 +29,7 @@ public class IndexPredicateFilter implements PredicateFilter { @NotNull @Override public List filter(@NotNull DataInstance sourceInstance, @NotNull TreeReference nodeSet, @NotNull XPathExpression predicate, @NotNull List children, @NotNull EvaluationContext evaluationContext, @NotNull Supplier> next) { - if (sourceInstance.getInstanceId() == null || isNested(nodeSet) || !(predicate instanceof XPathEqExpr)) { + if (sourceInstance.getInstanceId() == null || !(predicate instanceof XPathEqExpr)) { return next.get(); } @@ -52,16 +52,6 @@ public List filter(@NotNull DataInstance sourceInstance, @NotNull } } - private static boolean isNested(TreeReference nodeSet) { - for (int i = 1; i < nodeSet.size(); i++) { - if (nodeSet.getMultiplicity(i) > -1) { - return true; - } - } - - return false; - } - private void buildIndex(DataInstance sourceInstance, CompareChildToAbsoluteExpression predicate, List children, EvaluationContext evaluationContext, String section) { for (int i = 0; i < children.size(); i++) { TreeReference child = children.get(i); diff --git a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java index 6965e2d62..79772eb8f 100644 --- a/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java +++ b/src/main/java/org/javarosa/core/model/condition/EvaluationContext.java @@ -334,7 +334,7 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so for (int i = 0; i < predicates.size(); i++) { List filterChain; - if (i == 0) { + if (i == 0 && !isNested(nodeSetRef)) { filterChain = predicateFilterChain; } else { filterChain = DEFAULT_PREDICATE_FILTER_CHAIN; @@ -362,6 +362,16 @@ private void expandReferenceAccumulator(TreeReference sourceRef, DataInstance so } } + private static boolean isNested(TreeReference nodeSet) { + for (int i = 1; i < nodeSet.size(); i++) { + if (nodeSet.getMultiplicity(i) > -1) { + return true; + } + } + + return false; + } + @NotNull private List filterWithPredicate(DataInstance sourceInstance, TreeReference treeReference, XPathExpression predicate, List children, List filterChain) { return filterWithPredicate(sourceInstance, treeReference, predicate, children, 0, filterChain); From f08c2b310df5df1a717872ad0780a8437dedfc68 Mon Sep 17 00:00:00 2001 From: Callum Stott Date: Fri, 12 May 2023 13:41:59 +0300 Subject: [PATCH 47/47] Add test to check we aren't increasing load time when indexing --- .../core/model/test/PredicateCachingTest.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java index e69f1c6df..daa19c9e9 100644 --- a/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java +++ b/src/test/java/org/javarosa/core/model/test/PredicateCachingTest.java @@ -7,7 +7,9 @@ import static java.util.Arrays.asList; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.not; import static org.javarosa.core.util.BindBuilderXFormsElement.bind; import static org.javarosa.core.util.XFormsElement.body; import static org.javarosa.core.util.XFormsElement.group; @@ -568,4 +570,39 @@ public void repeatsUsedInCalculatesStayUpToDate() throws Exception { assertThat(scenario.answerOf("/data/result").getValue(), equalTo("70")); } + + @Test + public void eqPredicatesDoNotIncreaseLoadTime() { + int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> { + try { + Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("choice"), + t("calculate1"), + t("calculate2") + )), + instance("instance", + item("a", "A"), + item("b", "B") + ), + bind("/data/choice").type("string"), + bind("/data/calculate1").type("string") + .calculate("instance('instance')/root/item[value = /data/choice]/label") + ) + ), + body( + input("/data/choice") + ) + )); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + ); + + assertThat(evaluations, not(greaterThan(2))); + } }