Avoid multiple append calls for native functions in FunctionToString

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
Improves this µ-benchmark from 360ms to 320ms for me:

    var q = 0;
    var t = Date.now();
    for (var i = 0; i < 1000000; ++i) {
        q += Math.max.toString().length;
    }
    print(Date.now() - t, q);
Assignee

Comment 1

2 years ago
This simply replaces the three append() calls with a single call for the sourceless and native code cases. I've also replaced the two calls with a single-char string, because it seems the other overloaded append() method is slightly faster, but it probably doesn't really matter, since this code isn't executed when Function.prototype.toString is called.
Attachment #8886546 - Flags: review?(jdemooij)
Assignee

Comment 2

2 years ago
This patch removes the AppendPrelude lambda and inlines the code into the |!haveSource| branch. It also moves |out.append('(')| into the |haveSource| branch to ensure it's only called when haveSource==true (JSScript::loadSource may have set haveSource to false).
Attachment #8886548 - Flags: review?(jdemooij)
Assignee

Comment 3

2 years ago
Comment on attachment 8886548 [details] [diff] [review]
bug1380962-part2-remove-lambda.patch

Review of attachment 8886548 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfun.cpp
@@ +1039,5 @@
>          return nullptr;
>      }
>  
> +    if (haveSource) {
> +        bool funIsNonArrowLambda = fun->isLambda() && !fun->isArrow();

I guess we could also change this to:

    // If we're not in pretty mode, put parentheses around lambda functions
    // so that eval returns lambda, not function statement.
    bool addParentheses = !prettyPrint && (fun->isLambda() && !fun->isArrow());

so we only need to call isLambda() and isArrow() when we're not in pretty print mode.
Comment on attachment 8886546 [details] [diff] [review]
bug1380962-part1-merge-append.patch

Review of attachment 8886546 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8886546 - Flags: review?(jdemooij) → review+
Comment on attachment 8886548 [details] [diff] [review]
bug1380962-part2-remove-lambda.patch

Review of attachment 8886548 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense but a question below for something that isn't obvious to me.

::: js/src/jsfun.cpp
@@ -1037,5 @@
>  
> -    // If we're not in pretty mode, put parentheses around lambda functions
> -    // so that eval returns lambda, not function statement.
> -    if (haveSource && !prettyPrint && funIsNonArrowLambda) {
> -        if (!out.append('('))

Since loadSource below can set haveSource to |false|, why is it okay to move this check after that, into the haveSource branch? Shouldn't there be similar code in the !haveSource branch to account for this case?
Or did the old code have a bug here? It would be nice to have a shell test for this if possible.
Assignee

Comment 7

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #6)
> Or did the old code have a bug here? It would be nice to have a shell test
> for this if possible.

Yes, there was a bug in the old code:

js> setDiscardSource(true);
js> var f = function(){ return 0; }   
js> f.toSource()
"(function() {\n    [sourceless code]\n}"    <--- closing parenthesis missing here

with the patch applied, |f.toSource()| should print "function() {\n    [sourceless code]\n}", but we can also change it to "(function() {\n    [sourceless code]\n})", if you prefer that. Well, it seems like we add parentheses for asm-modules [1], so maybe we should also enclose the function source with parentheses for normal, sourceless functions.

[1] http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/js/src/wasm/AsmJS.cpp#8910-8911
Comment on attachment 8886548 [details] [diff] [review]
bug1380962-part2-remove-lambda.patch

Review of attachment 8886548 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I don't care too much what we do for sourceless functions since the current code was buggy and it's not exposed to the web.

::: js/src/jsfun.cpp
@@ +1039,5 @@
>          return nullptr;
>      }
>  
> +    if (haveSource) {
> +        bool funIsNonArrowLambda = fun->isLambda() && !fun->isArrow();

I agree with your addParentheses suggestion, it would simplify the code a bit.
Attachment #8886548 - Flags: review?(jdemooij) → review+
Assignee

Comment 9

2 years ago
Updated patch to add parentheses around sourceless functions. Carrying r+ from :jandem.


(In reply to Jan de Mooij [:jandem] from comment #8)
> Looks good. I don't care too much what we do for sourceless functions since
> the current code was buggy and it's not exposed to the web.

I've decided I go with adding parentheses around sourceless functions, so we're consistent with asm.js modules source representation. And added a test case for the sourceless string representation.
Attachment #8886548 - Attachment is obsolete: true
Attachment #8887375 - Flags: review+

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd687e1fe76d
Part 1: Merge append calls when assembling the source string for native functions. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c7ae816bdcc
Part 2: Remove unnecessary lambda function in FunctionToString. r=jandem
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd687e1fe76d
https://hg.mozilla.org/mozilla-central/rev/6c7ae816bdcc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.