Closed Bug 205703 Opened 21 years ago Closed 21 years ago

Refcount ExprResults

Categories

(Core :: XSLT, defect)

Product:
Core
Component:
XSLT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

Details

(Keywords: perf)

Attachments

(3 files, 3 obsolete files)

grep "()->get" recycle.diff > evil-argument.grep
5.88 KB, text/plain
Details
XSLTBench results
5.66 KB, text/plain
Details
patch v3
249.19 KB, patch
sicking
: review+
peterv
: superreview+
Details | Diff | Splinter Review 
Right now we end up cloning txAExprResults whenever variables are used. This is
pretty expensive for StringResults and NodeSets. I have a patch that refcounts
ExprResults (and renames it to txAExprResult) and refcounts them, which removes
the need to clone the result. It also removes the need to produce new
ExprResults in literal expressions like 'foo' or 9.

Since I had to change the signature of Expr::evaluate I took the opportunity to
make it return nsresult. This is by no means an attempt to cover all
errorhandling in XPath, but takes us a fair way in the right direction.

I'm also looking into creating a recycler to reuse ExprResult objects, but I'm
still looking into if this is worth it
Attached patch Patch v1 (obsolete) — — Splinter Review 
This patch makes us refcount ExprResults and creates a class to recycle them.
It recycles NodeSets, StringResults and NumberResults. The reason why
Boolresults aren't recycled is that we only create two of them and then only
use those two.

I didn't recycle RTFs since they are created somewhat differently. It should be
pretty easy to add that ability though, let me know if you want me to. I also
renamed createRTF to getAsRTF since the function can only be called once.
I have a few issues.
You use .get() too often. It should only be used to resolve ambiguities. If you 
think that you should have a method to return already_AddRefed, make an extra one.
Call it 
do_GetFooResult(aContext or aRecycler, aValue).

There are some inconsistencies about null checks.

The FunctionCall::evaluateTo should return nsresults, too. We don't wanna loose
them anymore. (They are not null safe, too, AFAICT.)

I see quite a few
nsRefPtr<txAExprResult> foo;
NS_STATIC_CAST(txAExprResult*, foo);
Is that a bug or what? Compiler bug, even? If it is, couldn't you get along 
with just a get()?
Here it would be used to resolve ambiguity, so I'd be fine with it.

Comments end in */, you change that back to **/ in at least one place.

Why do exprresults own their recylcer? Sounds strange to me.

I could use a pointer (and thus a comment) how ::evaluate may end up in a 
shared nodeset. I guess keys, but is that all?

Once this stabilizes (it might be stable enough already), I'd like to see perf
comparisons on a variety of testcases.
> You use .get() too often. It should only be used to resolve ambiguities. If you
> think that you should have a method to return already_AddRefed, make an extra
> one.
> Call it do_GetFooResult(aContext or aRecycler, aValue).

.get() is only used to resolve ambiguities on nsCOMPtrs. On already_AddRefed
it's the only way to be able to assign into a raw pointer which I don't see any
harm in.

> There are some inconsistencies about null checks.

Where?

> The FunctionCall::evaluateTo should return nsresults, too. We don't wanna loose
> them anymore. (They are not null safe, too, AFAICT.)

I planned to do the ::evaluateToXXX in a subsequent patch to keep the patchsize
down. Do you want me to do it anyway?

Not nullsafe where?

> I see quite a few
> nsRefPtr<txAExprResult> foo;
> NS_STATIC_CAST(txAExprResult*, foo);
> Is that a bug or what? Compiler bug, even? If it is, couldn't you get along
> with just a get()?

.get() would not help, but the plan is to change nsRefPtr and nsCOMPtr so that
calling .get() will help.

> Why do exprresults own their recylcer? Sounds strange to me.

Otherwise we'd crash if an ExprResult outlived the recycler. We would have to be
relying on things dying in a specific order when the txExecutionState goes away
(i.e. the recycler has to die last), and if we ever return ExprResults in XPCOM
objects we would never know what dies in what order.

> I could use a pointer (and thus a comment) how ::evaluate may end up in a 
> shared nodeset. I guess keys, but is that all?

By contract there is nothing that says weather ::evaluate returns a shared or
nonshared object. The three cases I can think of offhand when a shared object is
returned are keys, variables and literal-expressions, of which (currently) the
first two can return nodesets.


The problem with turning

already_AddRefed<StringResult> getStringResult(nsAString&);

into

nsresult getStringResult(nsAString&, StringResult**);
already_AddRefed<StringResult> do_GetStringResult(nsAString&, txResultRecycler*);

is that it won't allow you to assign directly into a raw-pointer, such as
aResult. I.e.

return aContext->recycler()->getStringResult(value, aResult);

would not compile.

I could (at least for most part) get around this by using the following signatures:

nsresult getStringResult(nsAString&, txAExprResult**);
already_AddRefed<txAExprResult> do_GetStringResult(nsAString&,txResultRecycler*);
nsresult getStringResult(StringResult**);
already_AddRefed<StringResult> do_GetStringResult(txResultRecycler*);

I.e. return an txAExprResult when the value is supplied and a StringResult when
no value is supplied.

However that is a bit inconsistent, and it also won't catch all cases.
Status: NEW → ASSIGNED
Oh, and that problem is not specific to StringResults, the same issue exsists
for all result-types
Lacking this:

operator alreadyAddRefed<T>*() const
        {
         return get();
         }

directly doing the .get() looks right to me. 
Alternative would be to add this operator?  
On 

nsresult getStringResult(nsAString&, txAExprResult**);
already_AddRefed<txAExprResult> do_GetStringResult(nsAString&, txResultRecycler*);
nsresult getStringResult(nsAString&, StringResult**);
already_AddRefed<StringResult> do_GetStringResult(nsAString&, txResultRecycler*);

this problem should have popped up with the current methods returning 
already_AddRefed already. I bet the getStringResult signatures just need the
nsresult getStringResult(nsAString&, txAExprResult**);
signature, as we hardly use the actual string without putting it into a refptr.

Wether the operator cast (or whatever you call it) is missing, that might be 
intentional, as returning addrefed raw pointers is considered bad. And that
would be a mockup for that. Not sure why this is considered bad, though.

It might be worth going for
nsresult getStringResult(nsAString&, txAExprResult**);
already_AddRefed<StringResult> do_GetStringResult(nsAString&, txResultRecycler*);
as signatures, I guess. At least no obvious usecases come to my mind that 
suggest otherwise.
we did some more talk on this.

The reason why the casting isn't a problem the way the patch is done now is that
when you return the value (rather then using an out-param) it can downcast
automatically, so:

*aResult = aContext->recycler()->getStringResult(foo).get();

works just fine since the StringResult* is downcast to an txAExprResult*.

One solution to this is to have multiple functions that differ only in
out-param-type. So we'd have

nsresult getStringResult(const nsAString&, txAExprResult**);
nsresult getStringResult(const nsAString&, StringResult**);
nsresult getStringResult(txAExprResult**);
nsresult getStringResult(StringResult**);

and then corresponding do_Get.. version.

Of course, this gets to be a whole lot of functions...
...though not all of them actually needs to be implemented since not all of them
would be used at this point.
As we don't get happy with the problem of the signature of the recycler, I did
a grep on the diff just to find out what we (or Jonas) (yikes, balancing blame
and fame is tricky) actually do with the stuff. I smell that a mere
getFooResult([value, ] txAExprResult**);
might actually do the trick. All that it makes Jonas pay is that we need a
nsRefPtr<foo> bar;
recycler()->getFooResult(... getter_AddRefs(bar));
instead of 
bar = recycler()->getFooResult.
The grep might show how common that is.
[D:\]grep -c "()->get" recycle.diff
76

nsRefPtr<foo> bar;
recycler()->getFooResult(... getter_AddRefs(bar));

won't compile if getFooResult has txAExprResult** as outargument
Just making sure that I understand... we are in agreement the pointer casting
being done with .get() is, execution-wise, exactly what is intended, right? ...
and so, most of the the discussion here about .get() is only about how to make
that 'more pretty' in the source code? 

I know I am 'old school' but....
all method overloading looks obtuse and error prone, and get() is
straightforward (in context of this particular code). If its really desired to
just reduce the instances of .get() appearing in source code, then how about a
macro? 
sam, the issue is to find the right result type so that the code works as designed.
Having a macro do cover something up would be the worst of all possible hacks,
IMHO.
Comment on attachment 123932 [details] [diff] [review]
Patch v1

I know you hate it, but txLiteralExpr::toString doesn't quote literals anymore.
so much for the txLiteralExpr.cpp and the death of Number and StringExpr.cpp
Attached file XSLTBench results — 
Results from three runs with the patch and three runs without the patch. On
average it seems like the patch reduces processing time with 20%
fixed comment 15 locally and the missing out-of-memory-check in AVT locally
Comment on attachment 123932 [details] [diff] [review]
Patch v1

on txResultRecycler, I'm not convinced about getEmptyStringResult. And I'm
still not happy with the return types. I'd prefer NS_IF_ADDREF and mere returns
of the pointer instead of else clauses and the explicit construction of the
already_AddRefed. I wonder if we should recycle all results, or just keep a
buffer of, say, N. 16. 128. Not sure what a reasonable number was. The recycled
ExprResults should clear their values on going in to the recycler, btw, not on
the way out. That should keep down the memory consumption, too.
I kinda fear us having a zoo of signatures.
Comment on attachment 123932 [details] [diff] [review]
Patch v1

getNumberResult should have a 
nsresult getNumberResult(Double aNumber, ExprResult**  aResult)
signature. Thus
+    *aResult = aContext->recycler()->getNumberResult(result).get();
+    NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);
+
+    return NS_OK;
would turn into
+    return aContext->recycler()->getNumberResult(result, aResult);

Same for getBoolResult, and prolly getEmptyStringResult().

tststs, ExprResult.h
     /**
      * Returns a pointer to the stringvalue if possible. Otherwise null is
      * returned.
-     */
+    **/
     virtual nsAString* stringValuePointer() = 0;

MultiplicativeExpr.cpp:
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    double rightDbl = exprRes->numberValue();
+    rv = leftExpr->evaluate(aContext, getter_AddRefs(exprRes));

weird empty lines. rightDbl is not part of leftExpr->evaluate

don't add uses of NS_ERROR_XPATH_INVALID_ARG,  please replace it with
NS_ERROR_XPATH_BAD_ARGUMENT_COUNT
for argument count checks.
Add it to xslt.properties, too.

PathExpr.cpp:
+    if (res->getResultType() != txAExprResult::NODESET) {
	 //XXX ErrorReport: report nonnodeset error
+	 return NS_ERROR_XSLT_NODESET_EXPECTED;

no XXX anymore.

RelationalExpr::compareResults
linelength of the signature.

UnionExpr::evaluate
+	 if (exprResult->getResultType() != txAExprResult::NODESET) {
+	     //XXX ErrorReport: report nonnodeset error
+	     return NS_ERROR_XSLT_NODESET_EXPECTED;

no XXX

VariableRefExpr::evaluate
     if (NS_FAILED(rv)) {
       // XXX report error, undefined variable
-      return new StringResult(NS_LITERAL_STRING("error"));
+      return rv;
no XXX, just NS_ENSURE_SUCCESS

I guess txOwningNodeSetContext should die now.

So much for /xpath/
I wonder if we should have getNodeSet and getString on txAExprResult. Those
would return nsnull if the result type is wrong, and this otherwise. Then
a test for NodeSet would look like
nsRefPtr<NodeSet> nodes = aResult->getNodeSet();
NS_ENSURE_TRUE(nodes, NS_ERROR_XSLT_NODESET_EXPECTED);

It would get around a bunch of casting problems all over the place as well.
No more NS_STATIC_CAST(NodeSet*, NS_STATIC_CAST(txAExprResult*, aFoo)); but
just a aFoo->getNodeSet();
I'd make them return non-AddRef'd mere pointers, so an assignment into nsRefPtr
does the right thing while you can still pass it into arguments or so that
want an Foo*. You just have to ADDREF if you keep a strong reference in a 
regular pointer. But I don't expect that to be too common.
Comment on attachment 123932 [details] [diff] [review]
Patch v1

rtfHandler->getAsRTF sounds like you could do that more than once to me.
getRTF?

txInstructions.cpp

@@ -81,13 +81,13
don't add whitespace

txPushNewContext::execute
no
	 // XXX ErrorReport: nodeset expected

txSetVariable::execute
+    rv = aEs.bindVariable(mName, exprRes);
+    NS_ENSURE_SUCCESS(rv, rv);

     return NS_OK;

either just return aEs.bindVariable, or
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "couldn't bind variable");
return rv;

The NS_ENSURE adds a void if to non-debug code.

Might appear elsewhere in the patch, too.

txVariableMap::getVariable
should work like
rv = getVariabe(aName, aResult);
you use the already_AddRefed exactly once.

txKeyPattern::matches
+    nsresult rv = es->getKeyNodes(mName, contextDoc, mValue, PR_TRUE,
getter_AddRefs(nodes));
+    if (NS_FAILED(rv))
	 return MB_FALSE;

do we want a 
NS_ENSURE_SUCCESS(rv, MB_FALSE); 
here?

txStepPattern::matches
put the 
+    nsresult rv = NS_OK;
+
down to where you use it.

+	     rv = predicate->evaluate(&predContext,
getter_AddRefs(exprResult));
+	     if (NS_FAILED(rv))
		 break;

should be a NS_ENSURE_SUCCESS(rv, MB_FALSE);
The break; tries to evaluate the next predicate, still.
The rv of the last predicate should ENSURE, too.

DocumentFunctionCall::evaluate should return a NS_ERROR_UNEXPECTED, I guess.
Function- and ElementAvailableFunctionCall, too.


Did you remove the string type check on the AvailableFunctionCalls on purpose?
I'm thru with this patch, still open are the signatures of getStringResult and 
getNodeSet. I'd need to sweep the patch again. Or Jonas could do so, too.

Oh, line lengths are a general issue, too, didn't bother giving each occurence.
on the matter of NS_ERROR_XPATH_INVALID_ARG, I'd vote for having a 
#define TX_ERROR_XXX NS_ERROR_FAILURE
if you wanna add dummy error values. Those are obviously values that should be
replaced by something reasonable, much more than NS_ERROR_XPATH_INVALID_ARG would
be.
Comment 19

> I'd prefer NS_IF_ADDREF and mere returns of the pointer instead of else clauses
> and the explicit construction of the already_AddRefed

Fixed the explicit construction of the already_AddRefed. I can't use
NS_IF_ADDREF because that doesn't compile when AddRef doesn't return a value. I
decided to not have it return a value since the function is inlined and not
returning a value should save some code. Let me know if you want me to change this.

> I wonder if we should recycle all results, or just keep a buffer of, say, N.
> 16. 128. Not sure what a reasonable number was.

What would be the advantage of that?

> The recycled ExprResults should clear their values on going in to the recycler,
> btw, not on the way out. That should keep down the memory consumption, too.

Much of the speed that the recycler gives comes from the fact that we recycle
the buffers that the ExprResults hold on to, i.e. the string-buffer or the
nodeset-buffer.

Clearing on the way out won't make a difference memorywise since no buffers will
be freed. It's pretty much equivalent to clear on the way in or on the way out.
Clearing on the way out saves a tiny amout of work in cases when we set the
value on the way out, and when an object is never returned.

> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    double rightDbl = exprRes->numberValue();
> +    rv = leftExpr->evaluate(aContext, getter_AddRefs(exprRes));
> weird empty lines. rightDbl is not part of leftExpr->evaluate

We usually have an empty-line after NS_ENSURE_*. I've added another line after
the rightDbl though.

> don't add uses of NS_ERROR_XPATH_INVALID_ARG,  please replace it with
> NS_ERROR_XPATH_BAD_ARGUMENT_COUNT
> for argument count checks.
> Add it to xslt.properties, too.

Done

> no XXX anymore.

We still need to call the context and report exactly what failed and in what
expression. That's for later though.

> I guess txOwningNodeSetContext should die now.

I pondered that and even tried it and it works fine. However we do save a cycle
or two by not refcounting, 'owning', the nodeset. That works fine for contexts
(the forwarding-context lacks refcounting too) that lives on the stack. Let me
know which way you want to go.
>> I wonder if we should recycle all results, or just keep a buffer of, say, N.
>> 16. 128. Not sure what a reasonable number was.
>
>What would be the advantage of that?

None that I can think of.... there's no practical way the number becomes
exceedingly large in any one transform, and all of the recycles are discarded at
the end of each transform.
>> I wonder if we should recycle all results, or just keep a buffer of, say, N.
>> 16. 128. Not sure what a reasonable number was.
>
> What would be the advantage of that?

You had no problems with runaway conditions on the one hand. On the other hand,
you get rid of memory management in txStack in favour of a statically allocated
strongly typed array.

I'd prefer to see txOwningNodeSetContext die.
re comment 20, I'd rather fix the problem in nsCOMPtr/nsRefPtr then try to work
around it by adding functions that just casts.

re comment 21
getRTF/getAsRTF seems equal to me.

> txVariableMap::getVariable
> should work like
> rv = getVariabe(aName, aResult);

Done, though i don't return an errorcode since it's often not an error for the
variable to not exist.

> do we want a 
> NS_ENSURE_SUCCESS(rv, MB_FALSE); 
> here?
yes

> put the 
> +    nsresult rv = NS_OK;
> +
> down to where you use it.
I like keeping it up high in the cases where I have to keep it on it's own
anyway (i.e. the first use is inside a block) since that way we won't have to
move it in case we insert more code that needs it.

> DocumentFunctionCall::evaluate should return a NS_ERROR_UNEXPECTED, I guess.
> Function- and ElementAvailableFunctionCall, too.
No, it should return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT. Some day these functions
needs some serious renesting, but that day is not today :)

> Did you remove the string type check on the AvailableFunctionCalls on purpose?
Yes

Comment 26
> You had no problems with runaway conditions on the one hand. On the other hand,
> you get rid of memory management in txStack in favour of a statically allocated
> strongly typed array.

It would be slower to use a static array since then i'd have to go through that
array and check which objects are still in use and which are not. And we might
end up in a situation when all objects are in use (looked up in variables or
whatnot) and then the recycler would stop saving us cycles.

It should be rare that we use a huge number of exprresults at the same time and
then release them, which is the only thing that will cause the recycler to
contain a large number of objects. And if that happens chances are pretty good
that we will execute the same expression again, needing the same number of
objects again.
Attached patch Patch v2 (obsolete) — — Splinter Review 
Fixes all Pikes comments except the ones i've commented on and the
recycler-signatures
Attachment #123932 - Attachment is obsolete: true
Attached patch Patch v2B (obsolete) — — Splinter Review 
This uses different signatures, nothing else should have changed. Well.. i
think I found one bug in the patch that I fixed, let me know if you want me to
track that one down
Comment on attachment 124771 [details] [diff] [review]
Patch v2B

I guess this is the one we'll go with
Attachment #124771 - Flags: superreview?(peterv)
Attachment #124771 - Flags: review?(axel)
ran all bustertests without regressions or crashes
...on standalone that is, module was already checked
Comment on attachment 124771 [details] [diff] [review]
Patch v2B

r=axel@pike.org
Attachment #124771 - Flags: review?(axel) → review+
Comment on attachment 124771 [details] [diff] [review]
Patch v2B

>Index: resources/xslt.properties
>===================================================================

> 11 = An XSLT stylesheet does not have an XML mimetype:
>+11 = An XPath function was called with the wrong number of arguments.

12

> 
> LoadingError = Error loading stylesheet: %S

>Index: source/xpath/AdditiveExpr.cpp
>===================================================================

>+nsresult
>+AdditiveExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    double rightDbl = Double::NaN;
>-    ExprResult* exprRes = 0;
>+    *aResult = nsnull;

Add a blank line after this one.

>+    nsRefPtr<txAExprResult> exprRes;
>+    nsresult rv = rightExpr->evaluate(aContext, getter_AddRefs(exprRes));
>+    NS_ENSURE_SUCCESS(rv, rv);
...
>+    rv = leftExpr->evaluate(aContext, getter_AddRefs(exprRes));
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>+    double leftDbl = exprRes->numberValue();

Add a blank line after this one.

>Index: source/xpath/AttributeValueTemplate.cpp
>===================================================================

>+nsresult
>+AttributeValueTemplate::evaluate(txIEvalContext* aContext,
>+                                 txAExprResult** aResult)
> {
>+    *aResult = nsnull;

Add a blank line after this one.

>+    *aResult = strRes;
>+
>+    NS_ADDREF(*aResult);

We need swap on nsRefPtr :-/

>Index: source/xpath/BooleanExpr.cpp
>===================================================================

>+nsresult
>+BooleanExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    MBool lval = MB_FALSE;
>-    ExprResult* exprRes = 0;
>-    if ( leftExpr ) {
>-        exprRes = leftExpr->evaluate(aContext);
>-        if ( exprRes ) lval = exprRes->booleanValue();
>-        delete exprRes;
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/BooleanFunctionCall.cpp
>===================================================================

>+nsresult
>+BooleanFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/BooleanResult.cpp
>===================================================================

>+BooleanResult::BooleanResult(MBool boolean)

PRBool

>Index: source/xpath/ExprResult.h
>===================================================================

>+class txAExprResult : public TxObject {

Why not txExprResult?
And since you're touching this line (and others below), please move the brace
to the next line.

>@@ -88,56 +100,61 @@ public:
> 
>     /**
>      * Converts this ExprResult to a Number (double) value
>      * @return the Number value
>     **/
>     virtual double numberValue()          = 0;
>+    
>+    /**
>+     * returns true if this object is owned by more then one owner

Returns ... than

>Index: source/xpath/FilterExpr.cpp
>===================================================================

>+nsresult
>+FilterExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    if (!aContext || !expr)
>-        return new NodeSet;
>+    *aResult = nsnull;

Add a blank line after this one.
expr can't be null anymore?

>+    if (nodes->isShared()) {
>+        // No need to hold a strong reference here since we
>+        // know that someone else is keeping this set alive.
>+        NodeSet* oldSet = nodes;
>+        rv = aContext->recycler()->getNodeSet(oldSet, getter_AddRefs(nodes));
>+        NS_ENSURE_SUCCESS(rv, rv);
>     }

I wonder if it wouldn't make sense to move this into the recycler as a function
instead of the isShared.

>Index: source/xpath/FunctionCall.cpp
>===================================================================

>+nsresult
>+FunctionCall::evaluateToNodeSet(Expr* aExpr, txIEvalContext* aContext, NodeSet** aResult)
> {
>     NS_ASSERTION(aExpr, "Missing expression to evaluate");
>-    ExprResult* exprResult = aExpr->evaluate(aContext);
>-    if (!exprResult)
>-        return 0;
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/LocationStep.cpp
>===================================================================

>+nsresult
>+LocationStep::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>     NS_ASSERTION(aContext, "internal error");
>-
>-    NodeSet* nodes = new NodeSet();
>-    if (!nodes)
>-        return 0;
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/MultiplicativeExpr.cpp
>===================================================================

>+nsresult
>+MultiplicativeExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    double rightDbl = Double::NaN;
>-    ExprResult* exprRes = 0;
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/NodeSetFunctionCall.cpp
>===================================================================

>                 case NAME:
>                 {
>+                    StringResult* strRes = nsnull;
>+                    rv = aContext->recycler()->getStringResult(&strRes);
>+                    *aResult = strRes;
>+                    NS_ENSURE_SUCCESS(rv, rv);
>+
>                     switch (node->getNodeType()) {
>                         case Node::ATTRIBUTE_NODE:
>                         case Node::ELEMENT_NODE:
>                         case Node::PROCESSING_INSTRUCTION_NODE:
>                         {
>                         // XXX Namespace: namespaces have a name
>-                            StringResult* result = new StringResult();
>-                            if (result) {
>-                                node->getNodeName(result->mValue);
>-                            }
>-                            return result;
>+                            node->getNodeName(strRes->mValue);
>                         }
>                         default:
>                         {
>                             break;

Do we want to call getEmptyStringResult in this case?

>Index: source/xpath/NumberFunctionCall.cpp
>===================================================================

>+nsresult
>+NumberFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>+    *aResult = nsnull;

Add a blank line after this one.

>+            if (Double::isNaN(dbl) || Double::isInfinite(dbl)) {
>+            }

I'd prefer if you negated this and moved the if and else below inside this one.

>+            else if (Double::isNeg(dbl) && dbl > -1) {
>+                dbl *= 0;
>+            }
>+            else {
>+                dbl = ceil(dbl);
>+            }
...
>+            if (Double::isNaN(dbl) || Double::isInfinite(dbl)) {
>+            }

Same here.

>+            else if (Double::isNeg(dbl) && dbl >= -0.5) {
>+                dbl *= 0;
>+            }
>+            else {
>+                dbl = floor(dbl + 0.5);
>+            }

>Index: source/xpath/NumberResult.cpp
>===================================================================

>+NumberResult::NumberResult(double aValue, txResultRecycler* aRecycler)
>+    : txAExprResult(aRecycler)
>+{
>+    value = aValue;

Member initializer please.

>Index: source/xpath/PathExpr.cpp
>===================================================================

>+nsresult
>+PathExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    if (!aContext || (expressions.getLength() == 0)) {
>-        NS_ASSERTION(0, "internal error");
>-        return new StringResult(NS_LITERAL_STRING("error"));
>-    }
>-
>-    NodeSet* nodes = new NodeSet(aContext->getContextNode());
>-    if (!nodes) {
>-        // XXX ErrorReport: out of memory
>-        NS_ASSERTION(0, "out of memory");
>-        return 0;
>-    }
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/PredicateList.cpp
>===================================================================

>+                case txAExprResult::NUMBER :

Remove the space before the colon.

>Index: source/xpath/RelationalExpr.cpp
>===================================================================

>+nsresult
>+RelationalExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    nsAutoPtr<ExprResult> lResult(mLeftExpr->evaluate(aContext));
>-    NS_ENSURE_TRUE(lResult, nsnull);
>-
>-    nsAutoPtr<ExprResult> rResult(mRightExpr->evaluate(aContext));
>-    NS_ENSURE_TRUE(rResult, nsnull);
>+    *aResult = nsnull;

Add a blank line after this one.

>Index: source/xpath/StringFunctionCall.cpp
>===================================================================

>+nsresult
>+StringFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>+    *aResult = nsnull;
>+    nsresult rv = NS_OK;

Add a blank line after this one.

>         case STARTS_WITH:
>         {
>             if (!requireParams(2, 2, aContext))
>-                return new StringResult(NS_LITERAL_STRING("error"));
>+                return NS_ERROR_XPATH_BAD_ARGUMENT_COUNT;
> 
>             nsAutoString arg1, arg2;
>             Expr* arg1Expr = (Expr*)iter.next();
>             evaluateToString((Expr*)iter.next(), aContext, arg2);
>-            if (arg2.IsEmpty())
>-                return new BooleanResult(PR_TRUE);
>+            if (arg2.IsEmpty()) {
>+                aContext->recycler()->getBoolResult(PR_TRUE, aResult);
>+            }
>+            else {
>+                evaluateToString(arg1Expr, aContext, arg1);
>+                aContext->recycler()->getBoolResult(arg1.Find(arg2) == 0, aResult);

Change .Find to StringBeginsWith

>@@ -173,101 +205,129 @@ ExprResult* StringFunctionCall::evaluate
...
>+            return aContext->recycler()->getStringResult(Substring(src, (PRUint32)start,
>+                                                                   (PRUint32)end -
>+                                                                   (PRUint32)start),

Make that (PRUint32)(end - start)

Please run this thru jst-review.pl and at least fix the long lines.
That's how far I got. Great patch, more coming tomorrow.
> expr can't be null anymore?

No, since i made the parser OOM safe we should never be ending up with
null-members in all our expressions.

>Index: source/xpath/NodeSetFunctionCall.cpp
> Do we want to call getEmptyStringResult in this case?

That won't really buy us anything and will cost extra code/binarysize

I'd prefer to keep the "A" in txAExprResult since this is still mostly an
abstract baseclass a'la nsAString. 
>>Index: source/xpath/NodeSetFunctionCall.cpp
>> Do we want to call getEmptyStringResult in this case?
> 
> That won't really buy us anything and will cost extra code/binarysize

Then I don't understand why we have getEmptyStringResult.
> > That won't really buy us anything and will cost extra code/binarysize

> Then I don't understand why we have getEmptyStringResult.

getEmptyStringResult pretty much only exists to keep the source cleaner. In many
cases (especially in StringFunctionCall) we need a StringResult containing an
empty string and with the getEmptyStringResult the code gets slightly cleaner.

Hmm.. at least that was the idea but looking at it it didn't really turn out
that way. Basically we can do one of three things:

1.
aContext->recycler()->getEmptyStringResult(aResult);

return NS_OK;

2.
return aContext->recycler()->
    getStringResult(NS_LITERAL_STRING(""), aResult);

3.
return aContext->recycler()->getEmptyStringResult(aResult);


Sourcewise I think 3 looks best, want me to go for that?
Comment on attachment 124771 [details] [diff] [review]
Patch v2B

>Index: source/xpath/StringResult.cpp
>===================================================================

>+StringResult::StringResult(const nsAString& str, txResultRecycler* aRecycler)
>+    : txAExprResult(aRecycler), mValue(str)

str -> aString or aValue

>Index: source/xpath/UnaryExpr.cpp
>===================================================================

>+nsresult
>+UnaryExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    ExprResult* exprRes = expr->evaluate(aContext);
>+    *aResult = nsnull;

Add a blank line.

>Index: source/xpath/VariableRefExpr.cpp
>===================================================================

>+nsresult
>+VariableRefExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    ExprResult* exprResult = 0;
>-    nsresult rv = aContext->getVariable(mNamespace, mLocalName, exprResult);
>+    nsresult rv = aContext->getVariable(mNamespace, mLocalName, *aResult);
>     if (NS_FAILED(rv)) {
>       // XXX report error, undefined variable
>-      return new StringResult(NS_LITERAL_STRING("error"));
>+      return rv;
>     }
>-    return exprResult->clone();
>+    return NS_OK;

Just return aContext->getVariable...

>Index: source/xpath/nsXPathEvaluator.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/transformiix/source/xpath/nsXPathEvaluator.h,v
>retrieving revision 1.9
>diff -u -6 -p -r1.9 nsXPathEvaluator.h
>--- source/xpath/nsXPathEvaluator.h	19 Apr 2003 00:28:07 -0000	1.9
>+++ source/xpath/nsXPathEvaluator.h	2 Jun 2003 23:42:46 -0000
>@@ -41,12 +41,14 @@
> #define nsXPathEvaluator_h__
> 
> #include "nsIDOMXPathEvaluator.h"
> #include "txIXPathContext.h"
> #include "nsIXPathEvaluatorInternal.h"
> #include "nsIWeakReference.h"
>+#include "nsAutoPtr.h"
>+#include "txResultRecycler.h"

No need for the second include anymore :-).

>Index: source/xpath/nsXPathExpression.cpp
>===================================================================

>-nsXPathExpression::nsXPathExpression(Expr* aExpression) : mExpression(aExpression)
>+nsXPathExpression::nsXPathExpression(Expr* aExpression,
>+                                     txResultRecycler* aRecycler)
>+    : mExpression(aExpression), mRecycler(aRecycler)

Move mRecycler on its own line.

>Index: source/xpath/txNodeSetContext.h
>===================================================================

>@@ -38,12 +38,13 @@
> 
> #ifndef __TX_XPATH_SET_CONTEXT
> #define __TX_XPATH_SET_CONTEXT
> 
> #include "txIXPathContext.h"
> #include "NodeSet.h"
>+#include "nsAutoPtr.h"

Why?

>Index: source/xpath/txResultRecycler.cpp
>===================================================================

>+txResultRecycler::txResultRecycler()
>+    : mEmptyStringResult(nsnull), mTrueResult(nsnull), mFalseResult(nsnull)

I prefer these on separate lines.

>Index: source/xpath/txResultRecycler.h
>===================================================================

>+class txResultRecycler
>+{

>+private:
>+    nsAutoRefCnt mRefCnt;
>+    txStack mStringResults;
>+    txStack mNodeSetResults;
>+    txStack mNumberResults;
>+    StringResult* mEmptyStringResult;
>+    BooleanResult* mTrueResult;
>+    BooleanResult* mFalseResult;

Make these nsRefPtr's?

>Index: source/xslt/txRtfHandler.h
>===================================================================

>+class txResultTreeFragment : public txAExprResult
> {
> public:
>-    txResultTreeFragment(txResultBuffer* aBuffer);
>+    txResultTreeFragment(nsAutoPtr<txResultBuffer> aBuffer);

nsAutoPtr<txResultBuffer>&?

>Index: source/xslt/txXSLTPatterns.cpp
>===================================================================

>+    nsRefPtr<NodeSet> nodes;
>+    nsresult rv = es->getKeyNodes(mName, contextDoc, mValue, PR_TRUE, getter_AddRefs(nodes));
>+    NS_ENSURE_SUCCESS(rv, MB_FALSE);

PR_FALSE

>     while (iter.hasNext()) {
>-        newNodes.clear();
>+        newNodes->clear();
>         MBool contextIsInPredicate = MB_FALSE;
>-        txNodeSetContext predContext(&nodes, aContext);
>+        txNodeSetContext predContext(nodes, aContext);
>         while (predContext.hasNext()) {
>             predContext.next();
>-            ExprResult* exprResult = predicate->evaluate(&predContext);
>-            if (!exprResult)
>-                break;
>+            nsRefPtr<txAExprResult> exprResult;
>+            rv = predicate->evaluate(&predContext, getter_AddRefs(exprResult));
>+            NS_ENSURE_SUCCESS(rv, PR_FALSE);
>+
>             switch(exprResult->getResultType()) {
>-                case ExprResult::NUMBER :
>+                case txAExprResult::NUMBER :

Remove the space before the colon.

>Index: source/xslt/functions/ElementAvailableFnCall.cpp
>===================================================================

>+nsresult
>+ElementAvailableFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    ExprResult* result = nsnull;
>-
>+    *aResult = nsnull;
>     if (requireParams(1, 1, aContext)) {

Return early.

>         txListIterator iter(&params);
>         Expr* param = (Expr*) iter.next();
>-        ExprResult* exprResult = param->evaluate(aContext);
>-        if (exprResult &&
>-            exprResult->getResultType() == ExprResult::STRING) {
>+
>+        nsRefPtr<txAExprResult> exprResult;
>+        nsresult rv = param->evaluate(aContext, getter_AddRefs(exprResult));

NS_ENSURE_SUCCESS(rv, rv) and de-indent the following lines.

>Index: source/xslt/functions/FunctionAvailableFnCall.cpp
>===================================================================

>+nsresult
>+FunctionAvailableFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    ExprResult* result = nsnull;
>-
>+    *aResult = nsnull;
>     if (requireParams(1, 1, aContext)) {

Return early.

>         txListIterator iter(&params);
>         Expr* param = (Expr*)iter.next();
>-        ExprResult* exprResult = param->evaluate(aContext);
>-        if (exprResult &&
>-            exprResult->getResultType() == ExprResult::STRING) {
>+        nsRefPtr<txAExprResult> exprResult;
>+        nsresult rv = param->evaluate(aContext, getter_AddRefs(exprResult));
>+        if (NS_SUCCEEDED(rv)) {

NS_ENSURE_SUCCESS(rv, rv) and de-indent the following lines.

>Index: source/xslt/functions/SystemPropertyFunctionCall.cpp
>===================================================================

>+nsresult
>+SystemPropertyFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)
> {
>-    ExprResult* result = nsnull;
>+    *aResult = nsnull;
> 
>     if (requireParams(1, 1, aContext)) {

Return early.

>         txListIterator iter(&params);
>         Expr* param = (Expr*)iter.next();
>-        ExprResult* exprResult = param->evaluate(aContext);
>-        if (exprResult->getResultType() == ExprResult::STRING) {
>+        nsRefPtr<txAExprResult> exprResult;
>+        nsresult rv = param->evaluate(aContext, getter_AddRefs(exprResult));
>+        if (NS_SUCCEEDED(rv)) {

NS_ENSURE_SUCCESS(rv, rv) and de-indent the following lines.

>             nsAutoString property;
>             exprResult->stringValue(property);
>             txExpandedName qname;
>             nsresult rv = qname.init(property, mMappings, MB_TRUE);
>-            if (NS_SUCCEEDED(rv) &&
>-                qname.mNamespaceID == kNameSpaceID_XSLT) {
>+            NS_ENSURE_SUCCESS(rv, rv);
>+
>+            if (qname.mNamespaceID == kNameSpaceID_XSLT) {
>                 if (qname.mLocalName == txXSLTAtoms::version) {
>-                    result = new NumberResult(1.0);
>+                    return aContext->recycler()->getNumberResult(1.0, aResult);
>                 }
>                 else if (qname.mLocalName == txXSLTAtoms::vendor) {

else after return.

>-                    result = new StringResult(NS_LITERAL_STRING("Transformiix"));
>+                    return aContext->recycler()->getStringResult(NS_LITERAL_STRING("Transformiix"), aResult);
>                 }
>                 else if (qname.mLocalName == txXSLTAtoms::vendorUrl) {

else after return.

>Index: source/xslt/functions/txFormatNumberFunctionCall.cpp
>===================================================================

>@@ -86,36 +88,37 @@ ExprResult* txFormatNumberFunctionCall::
>     value = evaluateToNumber((Expr*)iter.next(), aContext);
>     evaluateToString((Expr*)iter.next(), aContext, formatStr);
>     if (iter.hasNext()) {
>         nsAutoString formatQName;
>         evaluateToString((Expr*)iter.next(), aContext, formatQName);
>         rv = formatName.init(formatQName, mMappings, MB_FALSE);
>-        if (NS_FAILED(rv))
>-            formatName.mNamespaceID = kNameSpaceID_Unknown;
>+        NS_ENSURE_SUCCESS(rv, rv);

Why this change?
Comment on attachment 124771 [details] [diff] [review]
Patch v2B

>Index: source/xslt/util/txNodeSorter.cpp
>===================================================================

>@@ -222,49 +223,46 @@ nsresult txNodeSorter::sortNodeSet(NodeS
> int txNodeSorter::compareNodes(SortableNode* aSNode1,
>                                SortableNode* aSNode2,
>                                NodeSet* aNodes,
>                                txExecutionState* aEs)
> {

>         if (!aSNode1->mSortValues[i]) {
>             txForwardContext evalContext(aEs->getEvalContext(), aSNode1->mNode, aNodes);
>             aEs->pushEvalContext(&evalContext);
>-            ExprResult* res = key->mExpr->evaluate(&evalContext);
>+            nsRefPtr<txAExprResult> res;
>+            rv = key->mExpr->evaluate(&evalContext, getter_AddRefs(res));
>+            NS_ENSURE_SUCCESS(rv, rv);

This seems wrong. Return -1?

>         if (!aSNode2->mSortValues[i]) {
>             txForwardContext evalContext(aEs->getEvalContext(), aSNode2->mNode, aNodes);
>             aEs->pushEvalContext(&evalContext);
>-            ExprResult* res = key->mExpr->evaluate(&evalContext);
>+            nsRefPtr<txAExprResult> res;
>+            rv = key->mExpr->evaluate(&evalContext, getter_AddRefs(res));
>+            NS_ENSURE_SUCCESS(rv, rv);

Here too.
> Just return aContext->getVariable...

We should insert some errorreporting code there. So i'd rather keep stuff the
way they are.

> >+#include "nsAutoPtr.h"
> >+#include "txResultRecycler.h"
> No need for the second include anymore :-).

We're still up in the air on that one.

> >Index: source/xpath/txNodeSetContext.h
> Why?

Because I should make mNodeSet an nsRefPtr on request from Pike. Sorry forgot to
do that. Done now. Same in txForwardingContext. Though i'm still not 100% sure
it's a good idea.

> >Index: source/xpath/txResultRecycler.h
> Make these nsRefPtr's?

I can't, that gives circular includes :(

> >Index: source/xslt/functions/txFormatNumberFunctionCall.cpp
> >-        if (NS_FAILED(rv))
> >-            formatName.mNamespaceID = kNameSpaceID_Unknown;
> >+        NS_ENSURE_SUCCESS(rv, rv);
> Why this change?

Because that's IMHO the right way to fail. We used to fail trying to find the
named format (since no format will ever have a name with namespace
kNameSpaceID_Unknown). But the right way to fail is that the prefix is unknown
Attached patch patch v3 — — Splinter Review 
fixes petervs comments as well as making txNodeSetContext and txForwardContext
to refcount their container.
Attachment #124771 - Attachment is obsolete: true
Comment on attachment 125422 [details] [diff] [review]
patch v3

forwarding Pikes review and rerequesting sr
Attachment #125422 - Flags: superreview?(peterv)
Attachment #125422 - Flags: review+
Comment on attachment 125422 [details] [diff] [review]
patch v3

forwarding Pikes review and rerequesting sr
Comment on attachment 125422 [details] [diff] [review]
patch v3

yaiks! what happened here, removing duplicated flags. Review requests are still
there though :)
You didn't respond to

>+    if (nodes->isShared()) {
>+        // No need to hold a strong reference here since we
>+        // know that someone else is keeping this set alive.
>+        NodeSet* oldSet = nodes;
>+        rv = aContext->recycler()->getNodeSet(oldSet, getter_AddRefs(nodes));
>+        NS_ENSURE_SUCCESS(rv, rv);
>     }

I wonder if it wouldn't make sense to move this into the recycler as a function
instead of the isShared.


Comment on attachment 125422 [details] [diff] [review]
patch v3

>Index: source/xpath/BooleanExpr.cpp
>===================================================================

>+nsresult
>+BooleanExpr::evaluate(txIEvalContext* aContext, txAExprResult** aResult)

>+    nsRefPtr<txAExprResult> exprRes;
>+    nsresult rv =  leftExpr->evaluate(aContext, getter_AddRefs(exprRes));

Remove a space after the =.
Comment on attachment 125422 [details] [diff] [review]
patch v3

>Index: source/xpath/NodeSetFunctionCall.cpp
>===================================================================

>                 case NAME:
>                 {

>                         default:
>                         {
>-                            break;
>+                            aContext->recycler()->getStringResult(aResult);

getEmptyStringResult?
> I wonder if it wouldn't make sense to move this into the recycler as a function
> instead of the isShared.

Hmm.. i can't say i have any strong oppinions either way but I kind'a like this
way better. What signature would you have for the function if it lives in the
recycler?

nsresult getNonSharedNodeSet(NodeSet*, NodeSet**)?

nsresult getNonSharedResult(txAExprResult*, txAExprResult**)?

The latter would be slower, but the former would mean that we might end up with
getNonSharedFooResult too (though i think that's unlikly).

Fixed the two other issues locally
Comment on attachment 125422 [details] [diff] [review]
patch v3

>Index: source/xslt/txExecutionState.cpp
>===================================================================

>+txExecutionState::pushParamMap(txVariableMap* aParams)
> {
>     nsresult rv = mParamStack.push(mTemplateParams);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    mTemplateParams.forget();

    nsresult rv = mParamStack.push(mTemplateParams.forget());
    NS_ENSURE_SUCCESS(rv, rv);

>Index: source/xslt/functions/DocumentFunctionCall.cpp
>===================================================================

>+nsresult
>+DocumentFunctionCall::evaluate(txIEvalContext* aContext, txAExprResult** aResult)

>     // document(object, node-set?)
>     if (requireParams(1, 2, aContext)) {

Return early for !requireParams.

Either one of those getNonSharedNodeSet's is fine by me. It's only going to be
used in two places, so I don't care strongly.
Attachment #125422 - Flags: superreview?(peterv) → superreview+
wouldn't

nsresult rv = mParamStack.push(mTemplateParams.forget());

leak the mTemplateParams if the push failed?
Yep, that's why it is the way it is now. I'll do the other two changes though
Bah, the least you could do is compile a patch before posting it.

>Index: source/xslt/functions/FunctionAvailableFnCall.cpp
>===================================================================

>+    nsresult rv = param->evaluate(aContext, getter_AddRefs(exprResult));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsAutoString property;
>+    exprResult->stringValue(property);
>+    txExpandedName qname;
>+    nsresult rv = qname.init(property, mMappings, MB_FALSE);

Checked in. Thanks for reviews!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
This bug caused an increase in buildsize on linux of 22k. I suspect that most of
this comes from txAExprResult::Release being inside the .h file and therefor get
inlined everywhere. The fix for this is to move the implementation to a separate
.cpp file, the only question is which.

Should I create a new txAExprResult.cpp or add it to some existing .cpp file? I
don't care much other way (though the former sounds like the RightThing to me).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't like small c++ files from a compilation point of view.
I could live with having this function in txResultRecycler.cpp, too. It uses the
recycler, and with a comment in the header, it's kinda easy to find, too.
Assuming that the Release is really the cause.
moved ExprResult::Release to txResultRecycler, lets see if that reduces the bloat.
Marking fixed, the checkin reduced the binarysize by some 15k.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: