1380962 - Avoid multiple append calls for native functions in FunctionToString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

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);

Comment 1

[André Bargull [:anba]]

Attachment: bug1380962-part1-merge-append.patch

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.

Comment 2

[André Bargull [:anba]]

Attachment: bug1380962-part2-remove-lambda.patch

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).

Comment 3

[André Bargull [:anba]]

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 4

[Jan de Mooij [:jandem]]

Comment on attachment 8886546 [details] [diff] [review]
bug1380962-part1-merge-append.patch

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

Nice.

Comment 5

[Jan de Mooij [:jandem]]

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?

Comment 6

[Jan de Mooij [:jandem]]

Or did the old code have a bug here? It would be nice to have a shell test for this if possible.

Comment 7

[André Bargull [:anba]]

(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 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[André Bargull [:anba]]

Attachment: bug1380962-part2-remove-lambda.patch

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.

Comment 10

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d929d586209513f21fe082c7d03f2e4e97c43e68

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd687e1fe76d
https://hg.mozilla.org/mozilla-central/rev/6c7ae816bdcc