357345 - Further optimize XPath

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Patch comming up with the following optimizations:

1. Change "foo//bar" to "foo/descendant::bar". Also changes "foo//." to
   "foo/descendant-or-self::node()". Not sure if the latter is worth it, but
   it was just another 4 lines of code.

2. Changes "./*" to "*". This is mostly useful with the previous optimization
   since it changes ".//foo" to "descendant::foo"

3. Changes "foo | bar" to a single step with a union'ed txNodeTest. So in
   other words "child::foo | child::bar" is turned into "child::(foo | bar)".
   This should result in a lot less calls to
   txXPathNodeUtils::comparePosition which have shown up in profiles.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1 -w

A few comments:

I'm not exited about ownership pattern of txUnionNodeTest::addNodeTest (i.e. that it deletes upon failure), but it follows the pattern of PathExpr::addExpr and UnionExpr::addExpr.

I changed it so that different axis doesn't return different values for LocationStep::getType. Turns out that in many places i only care if an expression is a LocationStep or not, which was akward to do otherwise. I guess an alternative solution would be to add some function that checks for all possible location step types.

We should consider inlining more functions into txExpr.h

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 242823 [details] [diff] [review]
Patch v1 -w

>Index: content/xslt/src/xpath/txExpr.h
>===================================================================

>+#include "nsTArray.h"
>+#include "nsTPtrArray.h"

Including nsTPtrArray should be enough, no?

>@@ -834,48 +853,50 @@ class PathExpr : public Expr {

>+    void killExprAt(PRUint32 aPos);

Maybe name this deleteExprAt?

>+    PathOperator getPathOpAt(PRUint32 aPos)
>+    {
>+        NS_ASSERTION(aPos < mItems.Length(), "setting bad pathop index");

Getting?

>@@ -932,17 +953,22 @@ public:

>+    void killExprAt(PRUint32 aPos);

deleteExprAt?

>Index: content/xslt/src/xpath/txXPathOptimizer.cpp
>===================================================================

>+    if (subExpr->getType() == Expr::LOCATIONSTEP_EXPR &&
>+        (step = NS_STATIC_CAST(LocationStep*, subExpr))->
>+          getAxisIdentifier() == LocationStep::SELF_AXIS &&

I'd prefer if you moved the assignemnt of step out of the if (so have two nested if's).

>+        !step->getSubExprAt(0) &&
>+        path->getSubExprAt(1) &&
>+        path->getPathOpAt(1) != PathExpr::DESCENDANT_OP) {

>+txXPathOptimizer::optimizeUnion(Expr* aInExpr, Expr** aOutExpr)

>+        if (subExpr->getType() != Expr::LOCATIONSTEP_EXPR ||
>+            subExpr->getSubExprAt(0)) {
>+            continue;
>+        }

Add a newline here.

>+        LocationStep* currentStep = NS_STATIC_CAST(LocationStep*, subExpr);

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in.