Closed Bug 205703 Opened 22 years ago Closed 22 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: 22 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: 22 years ago → 22 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: