Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support optimizations for more expressions #727

Merged
merged 8 commits into from
Dec 5, 2023
Merged

Support optimizations for more expressions #727

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4554960
Support functions in CompareChildToAbsoluteExpression
seadowg Aug 16, 2023
81d8b0b
Only allow idempotent functions in CompareChildToAbsoluteExpression
seadowg Aug 16, 2023
c561210
Add test for function predicates
seadowg Aug 16, 2023
378e54b
Remove unneeded test code
seadowg Aug 17, 2023
d444802
Add support for current() expression caching
seadowg Aug 17, 2023
8770307
Add more testing to CompareChildToAbsoluteExpression
seadowg Aug 17, 2023
e86e64e
Remove commented out code
seadowg Dec 5, 2023
204ff22
Rename class and add docs
seadowg Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/main/java/org/javarosa/core/model/CompareChildToAbsoluteExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.javarosa.xpath.expr.XPathCmpExpr;
import org.javarosa.xpath.expr.XPathEqExpr;
import org.javarosa.xpath.expr.XPathExpression;
import org.javarosa.xpath.expr.XPathFuncExpr;
import org.javarosa.xpath.expr.XPathNumericLiteral;
import org.javarosa.xpath.expr.XPathPathExpr;
import org.javarosa.xpath.expr.XPathStringLiteral;
Expand Down Expand Up @@ -64,6 +65,10 @@ public static CompareChildToAbsoluteExpression parse(XPathExpression expression)
} else if (expression instanceof XPathEqExpr) {
a = ((XPathEqExpr) expression).a;
b = ((XPathEqExpr) expression).b;
} else if (expression instanceof XPathFuncExpr && expression.isIdempotent()
&& ((XPathFuncExpr) expression).args.length == 2) {
a = ((XPathFuncExpr) expression).args[0];
b = ((XPathFuncExpr) expression).args[1];
}

Pair<XPathPathExpr, XPathExpression> relativeAndAbsolute = getRelativeAndAbsolute(a, b);
Expand All @@ -81,10 +86,12 @@ private static Pair<XPathPathExpr, XPathExpression> getRelativeAndAbsolute(XPath
Queue<XPathExpression> 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 = subExpression;
if (subExpression instanceof XPathPathExpr) {
if (((XPathPathExpr) subExpression).init_context == XPathPathExpr.INIT_CONTEXT_RELATIVE)
relative = (XPathPathExpr) subExpression;
else {
absolute = subExpression;
}
} else if (subExpression instanceof XPathNumericLiteral || subExpression instanceof XPathStringLiteral) {
absolute = subExpression;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/javarosa/xpath/expr/XPathFuncExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,7 @@ public boolean containsFunc(@NotNull String name) {
return name.equals(id.name) || Arrays.stream(args).anyMatch(expression -> expression.containsFunc(name));
}

private static final String[] IDEMPOTENT_FUNCTIONS = new String[]{
public static final String[] IDEMPOTENT_FUNCTIONS = new String[]{
"regex",
"starts-with",
"ends-with",
Expand Down
79 changes: 79 additions & 0 deletions src/test/java/org/javarosa/core/model/CompareChildToAbsoluteExpressionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,41 @@
package org.javarosa.core.model;

import org.javarosa.xpath.expr.XPathEqExpr;
import org.javarosa.xpath.expr.XPathExpression;
import org.javarosa.xpath.expr.XPathFilterExpr;
import org.javarosa.xpath.expr.XPathFuncExpr;
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 java.util.Random;

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_doesNotParseExpressionsWhereBothSidesAreRelative() {
XPathEqExpr expression = new XPathEqExpr(
true,
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) }
),
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) }
)
);

CompareChildToAbsoluteExpression parsed = CompareChildToAbsoluteExpression.parse(expression);
assertThat(parsed, nullValue());
}

@Test
public void parse_parsesStringLiteralAsAbsolute() {
XPathEqExpr expression = new XPathEqExpr(
Expand Down Expand Up @@ -47,6 +68,64 @@ public void parse_parsesNumericLiteralAsAbsolute() {
assertThat(parsed.getAbsoluteSide(), equalTo(expression.b));
}

@Test
public void parse_parsesIdempotentFunctionWithAbsoluteAndRelativeArgs() {
String[] idempotentFunctions = XPathFuncExpr.IDEMPOTENT_FUNCTIONS;
XPathFuncExpr expression = new XPathFuncExpr(
new XPathQName(idempotentFunctions[new Random().nextInt(idempotentFunctions.length)]),
new XPathExpression[]{
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) }
),
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_ROOT, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("something")) }
),
}
);

CompareChildToAbsoluteExpression parsed = CompareChildToAbsoluteExpression.parse(expression);
assertThat(parsed, not(nullValue()));
assertThat(parsed.getRelativeSide(), equalTo(expression.args[0]));
assertThat(parsed.getAbsoluteSide(), equalTo(expression.args[1]));
}

@Test
public void parse_doesNotParseNonIdempotentFunction() {
XPathFuncExpr expression = new XPathFuncExpr(
new XPathQName("blah"),
new XPathExpression[]{
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) }
),
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_ROOT, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("something")) }
),
}
);

CompareChildToAbsoluteExpression parsed = CompareChildToAbsoluteExpression.parse(expression);
assertThat(parsed, nullValue());
}

@Test
public void parse_parsesContextRelativeExpressionsAsAbsolute() {
XPathEqExpr expression = new XPathEqExpr(
true,
new XPathPathExpr(XPathPathExpr.INIT_CONTEXT_RELATIVE, new XPathStep[]{
new XPathStep(XPathStep.AXIS_CHILD, new XPathQName("name")) }
),
new XPathPathExpr(
new XPathFilterExpr(new XPathFuncExpr(new XPathQName("blah"), new XPathExpression[0]), new XPathExpression[0]),
new XPathStep[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[]{
Expand Down
38 changes: 37 additions & 1 deletion src/test/java/org/javarosa/core/model/PredicateCachingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public void firstPredicateInMultipleEqPredicatesAreOnlyEvaluatedOnce() throws Ex
}

@Test
public void repeatedCompPredicatesWithSameAnswerAreOnlyEvaluatedOnce() throws Exception {
public void repeatedCompPredicatesWithSameAbsoluteValueAreOnlyEvaluatedOnce() throws Exception {
Scenario scenario = Scenario.init("Some form", html(
head(
title("Some form"),
Expand Down Expand Up @@ -255,6 +255,42 @@ public void repeatedCompPredicatesWithSameAnswerAreOnlyEvaluatedOnce() throws Ex
assertThat(evaluations, lessThan(4));
}

@Test
public void repeatedIdempotentFuncPredicatesWithSameAbsoluteValueAreOnlyEvaluatedOnce() throws Exception {
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[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(asList("PredicateEvaluation", "IndexEvaluation"), () -> {
scenario.answer("/data/choice", "a");
scenario.answer("/data/choice", "a");
});

// Check that we do less than size of secondary instance * number of times we answer
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
Expand Down
38 changes: 38 additions & 0 deletions src/test/java/org/javarosa/core/model/SelectCachingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,5 +209,43 @@ public void eqChoiceFilter_inRepeat_onlyEvaluatedOnce() throws Exception {
// Check that we do less than (size of secondary instance) * (number of choice lookups)
assertThat(evaluations, lessThan(8));
}

@Test
public void eqChoiceFiltersInRepeatsWithCurrentPathExpressionsAreOnlyEvaluatedOnce() throws Exception {
Scenario scenario = Scenario.init("Select in repeat", html(
head(
title("Select in repeat"),
model(
mainInstance(
t("data id='repeat-select'",
t("outer",
t("filter"),
t("inner",
t("select"))))),

instance("choices",
item("a", "A"),
item("b", "B")))),
body(
repeat("/data/outer",
input("filter"),
repeat("/data/outer/inner",
select1Dynamic("/data/outer/inner/select", "instance('choices')/root/item[value=current()/../../filter]"))
))));

scenario.answer("/data/outer[0]/filter", "a");
scenario.createNewRepeat("/data/outer[0]/inner");
scenario.answer("/data/outer[1]/filter", "a");
scenario.createNewRepeat("/data/outer[1]/inner");
scenario.createNewRepeat("/data/outer[1]/inner");

int evaluations = Measure.withMeasure(asList("PredicateEvaluation", "IndexEvaluation"), () -> {
scenario.choicesOf("/data/outer[0]/inner[0]/select");
scenario.choicesOf("/data/outer[0]/inner[1]/select");
});

// Check that we do less than (size of secondary instance) * (number of choice lookups)
assertThat(evaluations, lessThan(4));
}
//endregion
}
4 changes: 2 additions & 2 deletions src/test/java/org/javarosa/core/test/Scenario.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,8 +900,8 @@ public List<SelectChoice> choicesOf(String xPath) {
TreeReference reference = expandSingle(getRef(xPath));

FormEntryPrompt questionPrompt = model.getQuestionPrompt(getIndexOf(reference));
// This call triggers the correct population of dynamic choices.
questionPrompt.getAnswerValue();
// // This call triggers the correct population of dynamic choices.
// questionPrompt.getAnswerValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was definitely needed originally. I'm not sure at what point it stopped being required. Maybe when we started caching? Do we need to look deeper into it or can we just delete these two lines?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In v3.0.0, it was definitely needed

QuestionDef control = questionPrompt.getQuestion();
return control.getChoices() == null
// If the (static) choices is null, that means there is an itemset and choices are dynamic
Expand Down