Bug 14112 – Cross-port KDE KJS UnaryPlus and UnaryMinus optimizations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Luciano Montanaro]

The attached patch ports some code reorganization by Harry Porten and me with the simplest optimizations for javascript expressions like

var x = +1;
var y = -1;

for which one NumberNode is generated instead of Unary{Plus,Minus}Node -> NumberNode.

[Luciano Montanaro]

Created attachment 14992 [details]
The patch discussed in the bugzilla entry

[Darin Adler]

Comment on attachment 14992 [details]
The patch discussed in the bugzilla entry

There seem to be a lot of gratuitous whitespace changes in this patch. Did someone run a "remove trailing whitespace" script? If so, why?

All the changes to nodes.cpp seem to be whitespace only and unrelated to the fix.

Could we get separate patches for the reformatting and moving things around and making things into functions from the patch that actually has the optimization?

Is it acceptable to turn a unary plus into part of the number? It seems you'd lose the "+" from the serialization of the function.

[Luciano Montanaro]

Uhm, the nodes.cpp slipped in, as did the kjs_binding.h change, which I needed at sometime to build things.

The kjs_binding.h part should be removed, the nodes,cpp, I'd like to keep (should I send it as a different patch?), since it remove noise while comparing the two branches.

For the rest, I originally intended to submit a patch to split makenodes.h out of grammar.y, but I went on a bit further than that.

I can split up the patch, however I need input on how to do that.

In kjs, there has been the makenodes.h split and then the optimization.

Would patche in that order be ok?

About the optimization: are you concerned of debugging?
The unaryplus number -> number optimization is probably almost useless, but the unaryminus number pattern happens often enough.

Harri porten measured something like a 1.7 improvement over md5-1.js with a version of KJS including this patch and another one to convert number arithmeticoperator number -> number. But obviously the tree will be different from the original.

[Maciej Stachowiak]

Yes, patches in that order would be ok (as discussed on IRC). I will r- this for now, for the splitting. Given where we are in the project it might also be smarter to reserve this optimization for the feature branch, but I leave that to your judgment.

[Maciej Stachowiak]

Comment on attachment 14992 [details]
The patch discussed in the bugzilla entry

(nominal r- as stated above; please split patch)

[Cameron Zwarich (cpst)]

Created attachment 15864 [details]
Proposed patch

Here's a revised version of the patch that makes minimal extraneous changes.

[Darin Adler]

Comment on attachment 15864 [details]
Proposed patch

This doesn't seem like an important optimization. How often does this happen in practice? Also, it seems like it breaks serialization of functions.

+    if (n->isNumber())
+        return n;
+    else
+        return new UnaryPlusNode(n);

We normally don't do else after return.

The whitespace in the code above doesn't match our usual idiom. For example it should be this.

        NumberNode* number = static_cast<NumberNode*>(n);

Also:

+    double value() const KJS_FAST_CALL { return val; }
+    void setValue(double v) KJS_FAST_CALL { val = v; }

Do inline functions like these need KJS_FAST_CALL?

review- because of the "breaks serialization" issue. It's fine to optimize execution but somehow we have to record the original code so we can produce it on demand.

[Darin Adler]

(In reply to comment #7)
> (From update of attachment 15864 [details] [edit])
> This doesn't seem like an important optimization. How often does this happen in
> practice?

Maybe the "-1" case is more important and more worth fixing. Obviously that can be fixed without breaking serialization.

[Cameron Zwarich (cpst)]

(In reply to comment #7)
> (From update of attachment 15864 [details] [edit])
> This doesn't seem like an important optimization. How often does this happen in
> practice? Also, it seems like it breaks serialization of functions.

I'll remove the UnaryPlusNode optimization and keep the NegateNode one instead of adding a fix for this.

> +    if (n->isNumber())
> +        return n;
> +    else
> +        return new UnaryPlusNode(n);
> 
> We normally don't do else after return.

Noted.

> The whitespace in the code above doesn't match our usual idiom. For example it
> should be this.
> 
>         NumberNode* number = static_cast<NumberNode*>(n);
> 
> Also:
> 
> +    double value() const KJS_FAST_CALL { return val; }
> +    void setValue(double v) KJS_FAST_CALL { val = v; }
> 
> Do inline functions like these need KJS_FAST_CALL?

I don't know, but it is the current practice in nodes.h.

[Cameron Zwarich (cpst)]

Created attachment 15935 [details]
Revised proposed patch

[Cameron Zwarich (cpst)]

Created attachment 15936 [details]
Revised proposed patch

Oops. I forgot my last name in the ChangeLog entry.

[Darin Adler]

Comment on attachment 15936 [details]
Revised proposed patch

If the number is already negative, then I think this breaks serialization.

I could be wrong. We need a test case to demonstrate whether that is true or not.

[Cameron Zwarich (cpst)]

It does indeed cause serialization to not work like one would expect, but strictly speaking, where is the requirement stated that serialization must give exactly the same parse tree when reparsed? For example, Firefox 2.0 will serialize a - - 5 inside of a function as a 5. Is this violating the spec?

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> It does indeed cause serialization to not work like one would expect, but
> strictly speaking, where is the requirement stated that serialization must give
> exactly the same parse tree when reparsed? For example, Firefox 2.0 will
> serialize a - - 5 inside of a function as a 5. Is this violating the spec?

It violates the spirit of correctness, and it makes jsfunfuzz, well, less fun (and less useful).

http://www.squarefree.com/2007/08/02/introducing-jsfunfuzz/
http://www.squarefree.com/2007/08/02/fuzzing-for-correctness/

Jesse Ruderman may be interested in the "a - - 5" deserialization issue in Firefox.

[Cameron Zwarich (cpst)]

Created attachment 15957 [details]
Revised proposed patch

Those sound like good enough reasons. Here's a patch that fixes the serialization problem, along with test cases. The NaN case was never even an issue due to how NaN is handled in JavaScriptCore, but I added it just in case that ever changes.

[Jesse Ruderman]

Decompiling "a = - - 5" as "a = 5" is fine, since both statements do exactly the same thing.  SpiderMonkey developers refer to this and related optimizations as "constant folding".

Constant folding usually doesn't cause jsfunfuzz to complain, since jsfunfuzz only cares whether the decompiled function is canonical, not whether it roughly matches the original.  (An example where it might confuse jsfunfuzz is if "2*(3+4)" were to become "2*7" and then become "14" instead of just becoming "14" on the first trip.)

Note that turning "- - x" or "+ x" into "x" is not correct, because it loses a coercion-to-number.

If you add this optimization, be sure to test that you get zero vs. negative-zero right: "-0" should not just become "0".

[Cameron Zwarich (cpst)]

(In reply to comment #16)

> If you add this optimization, be sure to test that you get zero vs.
> negative-zero right: "-0" should not just become "0".

That's one of my test cases. ;-)

[Darin Adler]

Comment on attachment 15957 [details]
Revised proposed patch

+        if (number->value() < 0.0) {

I think this should be > 0.0 rather than < 0.0.

+        "- - 0",
+        "- - NaN"

I think we need test cases for -1 and for 1.

[Cameron Zwarich (cpst)]

Created attachment 15963 [details]
Revised proposed patch

Sorry, I really should have caught that. I guess I was tired. Here is the fixed version.

[Darin Adler]

Comment on attachment 15963 [details]
Revised proposed patch

r=me

[Mark Rowe (bdash)]

Landed in r26582.