Bug 14112 - Cross-port KDE KJS UnaryPlus and UnaryMinus optimizations
Summary: Cross-port KDE KJS UnaryPlus and UnaryMinus optimizations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Other All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-06-13 01:27 PDT by Luciano Montanaro
Modified: 2007-10-14 04:41 PDT (History)
3 users (show)

See Also:


Attachments
The patch discussed in the bugzilla entry (20.14 KB, patch)
2007-06-13 01:30 PDT, Luciano Montanaro
mjs: review-
Details | Formatted Diff | Diff
Proposed patch (4.52 KB, patch)
2007-08-07 23:24 PDT, Cameron Zwarich (cpst)
darin: review-
Details | Formatted Diff | Diff
Revised proposed patch (4.16 KB, patch)
2007-08-11 20:21 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Revised proposed patch (4.17 KB, patch)
2007-08-11 20:24 PDT, Cameron Zwarich (cpst)
darin: review-
Details | Formatted Diff | Diff
Revised proposed patch (5.92 KB, patch)
2007-08-13 23:44 PDT, Cameron Zwarich (cpst)
darin: review-
Details | Formatted Diff | Diff
Revised proposed patch (6.12 KB, patch)
2007-08-14 11:13 PDT, Cameron Zwarich (cpst)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luciano Montanaro 2007-06-13 01:27:16 PDT
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.
Comment 1 Luciano Montanaro 2007-06-13 01:30:22 PDT
Created attachment 14992 [details]
The patch discussed in the bugzilla entry
Comment 2 Darin Adler 2007-06-14 20:44:44 PDT
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.
Comment 3 Luciano Montanaro 2007-06-15 01:39:40 PDT
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.


Comment 4 Maciej Stachowiak 2007-06-25 20:19:19 PDT
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.
Comment 5 Maciej Stachowiak 2007-06-25 20:20:38 PDT
Comment on attachment 14992 [details]
The patch discussed in the bugzilla entry

(nominal r- as stated above; please split patch)
Comment 6 Cameron Zwarich (cpst) 2007-08-07 23:24:30 PDT
Created attachment 15864 [details]
Proposed patch

Here's a revised version of the patch that makes minimal extraneous changes.
Comment 7 Darin Adler 2007-08-11 15:38:43 PDT
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.
Comment 8 Darin Adler 2007-08-11 15:41:48 PDT
(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.
Comment 9 Cameron Zwarich (cpst) 2007-08-11 15:49:42 PDT
(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.
Comment 10 Cameron Zwarich (cpst) 2007-08-11 20:21:26 PDT
Created attachment 15935 [details]
Revised proposed patch
Comment 11 Cameron Zwarich (cpst) 2007-08-11 20:24:17 PDT
Created attachment 15936 [details]
Revised proposed patch

Oops. I forgot my last name in the ChangeLog entry.
Comment 12 Darin Adler 2007-08-13 11:41:59 PDT
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.
Comment 13 Cameron Zwarich (cpst) 2007-08-13 12:36:45 PDT
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?
Comment 14 David Kilzer (:ddkilzer) 2007-08-13 23:18:35 PDT
(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.

Comment 15 Cameron Zwarich (cpst) 2007-08-13 23:44:45 PDT
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.
Comment 16 Jesse Ruderman 2007-08-14 01:26:27 PDT
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".
Comment 17 Cameron Zwarich (cpst) 2007-08-14 01:30:02 PDT
(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. ;-)
Comment 18 Darin Adler 2007-08-14 07:57:33 PDT
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.
Comment 19 Cameron Zwarich (cpst) 2007-08-14 11:13:56 PDT
Created attachment 15963 [details]
Revised proposed patch

Sorry, I really should have caught that. I guess I was tired. Here is the fixed version.
Comment 20 Darin Adler 2007-08-14 15:35:05 PDT
Comment on attachment 15963 [details]
Revised proposed patch

r=me
Comment 21 Mark Rowe (bdash) 2007-10-14 04:41:46 PDT
Landed in r26582.