Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Use of Native Xpath - Faster calculate when big data #906

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use of Native Xpath - Faster calculate when big data #906

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3dd5d2
try to always use native approach to improve faster search when model…
Jun 10, 2022
7ae4caa
prettier fix
Jun 10, 2022
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
36 changes: 12 additions & 24 deletions src/js/form-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,30 +1561,18 @@ FormModel.prototype.evaluate = function (
}

// try native to see if that works... (will not work if the expr contains custom OpenRosa functions)
if (
tryNative &&
Copy link
Member

@MartijnR MartijnR Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note, it is unfortunately not safe to always try native because in this ecosystem we do things that can be evaluated by a native evaluator but return an incorrect result for ODK XForms. I'm surprised no tests failed though (or did they?).

The main (and perhaps only) things are comparisons and arithmetic with date/dateTime strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is a shame. I see a few other cases called out in the spec. This feels like something we could maybe revisit with the tree-sitter-xpath grammar, which is fast enough that we'd still see significant performance improvements.

Copy link
Member

@MartijnR MartijnR Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's a huge shame. We've had discussions about a turbo mode that requires form designers to wrap such strings with date and date-time.

Yes, indeed, good find. There are some native XPath 1.0. functions that have deviating behavior so some of those would also be an issue.

Curious about tree-sitter! Will it be able to find out if the value of /path/to/node is a date string?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alone, the tree-sitter grammar won't be able to do any kind of static type analysis, that's not available in the AST. Here's ways I'd imagine it could help for this case:

  • Rule out expressions with functions we know deviate (although we can skip some because they'll fail to compile, which should perform similarly).
  • Rule out expressions where we know an argument position is of date/date-time type.
  • Relate nodeset expressions to their bindings, to identify their type and rule out expressions with those nodesets1.
  • Rule out expressions with literals which would be treated as dates.

If all of this sounds like it has overlap with openrosa-xpath-evaluator's parsing responsibilities... it does, heh. But it would probably be a good fit for this case, because tree-sitter is exceptionally fast.

Footnotes

  1. This dovetails with other prototype work I've explored, identifying nodeset subexpressions to determine their dependencies. This already works for a huge set of expressions I pulled from openrosa-xpath-evaluator's tests, you can see the test fixtures used to validating that here. The grammar has proven pretty reliable so far. You can see example usage here and here to find nodeset sub-expressions. I have additional prototype work (currently only local) for resolving those sub-expressions to actual related nodes, which so far has worked for everything I've tried except with relative nested-subexpressions (e.g. in a predicate). ↩

Copy link
Member

Choose a reason for hiding this comment

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

Cool stuff!

I have additional prototype work (currently only local) for resolving those sub-expressions to actual related nodes, which so far has worked for everything

I'm guessing the challenge might be to prevent this from becoming too costly (possibly negate performance improvements achieved by sending to the native evaluator). Will be awesome if that is possible!

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s more than possible, it’s a reality! I’ll push up instructions for running and measuring the subexpression logic when I get a chance, but for now I’ll just say that finding subexpressions in all of the cases I pulled from the current evaluator test suite averages 1ms or less even when my computer is throttling under heavy load. In my local stress testing, the native evaluator is also generally ~1ms, the extended evaluator is generally 10+ms.

typeof doc.evaluate !== 'undefined' &&
!OPENROSA.test(expr)
) {
try {
// console.log( 'trying the blazing fast native XPath Evaluator for', expr, index );
result = doc.evaluate(
expr,
context,
this.getNsResolver(),
resTypeNum,
null
);
} catch (e) {
// console.log( '%cWell native XPath evaluation did not work... No worries, worth a shot, the expression probably ' +
// 'contained unknown OpenRosa functions or errors:', expr );
}
}

// if that didn't work, try the slow XPathJS evaluator
if (!result) {
try {
// console.log( 'trying the blazing fast native XPath Evaluator for', expr, index );
result = doc.evaluate(
expr,
context,
this.getNsResolver(),
resTypeNum,
null
);
} catch {
try {
if (typeof doc.jsEvaluate === 'undefined') {
if ( typeof doc.jsEvaluate === 'undefined' ) {
this.bindJsEvaluator();
}
// console.log( 'trying the slow enketo-xpathjs "openrosa" evaluator for', expr, index );
Expand All @@ -1595,7 +1583,7 @@ FormModel.prototype.evaluate = function (
resTypeNum,
null
);
} catch (e) {
} catch ( e ) {
throw new FormLogicError(
`Could not evaluate: ${expr}, message: ${e.message}`
);
Expand Down