Bug 158318 – Implement Air::allocateStack() in ES6 to see how much of a bad idea that is

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Filip Pizlo]

We would never actually meta-circulate our engine, because why, but it's useful to see how the kind of code we write would fare if it was written in ES6.  I want to know:

- Will it work at all?
- Will it run without crashing?
- How many times slower will it be than C++?

These are useful questions to answer, for science.

[Filip Pizlo]

Created attachment 280369 [details]
gotta start by dumping Air to a JS data definition

[Filip Pizlo]

Created attachment 280371 [details]
the C++ side may be written

Yucky!

[Filip Pizlo]

Created attachment 280447 [details]
moar

[Filip Pizlo]

Created attachment 280452 [details]
so much fun

[Filip Pizlo]

Created attachment 280456 [details]
a bit more

[Filip Pizlo]

Created attachment 280629 [details]
it grows

Intriguingly, I found some slop in the C++ implementation of Air by rewriting it.  This also includes changes to remove dead code and consolidate duplicated code.

[Filip Pizlo]

Created attachment 280642 [details]
a bit more

[Filip Pizlo]

Created attachment 280664 [details]
getting there

[Filip Pizlo]

Created attachment 280666 [details]
added customs

[Filip Pizlo]

Created attachment 280730 [details]
getting pretty close

[Filip Pizlo]

Created attachment 280733 [details]
almost done

[Filip Pizlo]

Created attachment 280735 [details]
the patch

[WebKit Commit Bot]

Attachment 280735 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:93:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:117:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:194:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:198:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:204:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirArgInlines.h:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirArgInlines.h:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 280735 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280735&action=review

I'm not going to read every line, but LGTM

> PerformanceTests/ChangeLog:21
> +        - JSAir's largest hot function, which is anonymous, so we call it by its hash (ACLj8C).

Maybe it's worth giving the anonymous function a name?

> PerformanceTests/JSAir/benchmark.js:1
> +"use strict";

I think it's nicer for this to go after copyright. It's less likely to be glanced over
Same goes for in all other files.

> PerformanceTests/JSAir/opcode.js:736
> +        break;
> +        break;

Many duplicate breaks

> PerformanceTests/JSAir/payload-gbemu-executeIteration.js:1
> +"use strict";

Should we generate a copyright?

> Source/JavaScriptCore/b3/air/AirDumpAsJS.h:37
> +// This is a joke. Various operations on Air are interesting from a benchmarking standpoint. We can

"This is a joke." => "This is used for benchmarking."

> Source/WTF/wtf/PrintStream.h:75
> +    void println(const Types&... values)

I will actually use this a lot

[Saam Barati]

Comment on attachment 280735 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280735&action=review

> PerformanceTests/ChangeLog:33
> +        - Symbol.
> +        - for-of.
> +        - arrow functions.
> +        - Map/Set.
> +        - let/const.
> +        - classes.

It's worth opening a bug to write a benchmark that uses generators.

[Filip Pizlo]

Created attachment 280737 [details]
almost the patch

Uploading this to check EWS, but I still need to address Saam's comments.

[WebKit Commit Bot]

Attachment 280737 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:93:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:117:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:194:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:198:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirDumpAsJS.cpp:204:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirArgInlines.h:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirArgInlines.h:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #14)
> Comment on attachment 280735 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280735&action=review
> 
> I'm not going to read every line, but LGTM
> 
> > PerformanceTests/ChangeLog:21
> > +        - JSAir's largest hot function, which is anonymous, so we call it by its hash (ACLj8C).
> 
> Maybe it's worth giving the anonymous function a name?

I'll try to check this.  It's probably an arrow function.

> 
> > PerformanceTests/JSAir/benchmark.js:1
> > +"use strict";
> 
> I think it's nicer for this to go after copyright. It's less likely to be
> glanced over
> Same goes for in all other files.

I wasn't sure if that worked or not.  I'll fix this.

> 
> > PerformanceTests/JSAir/opcode.js:736
> > +        break;
> > +        break;
> 
> Many duplicate breaks

Yeah, this is generated code.  The code generator isn't that smart.  It does the same thing when generating C++ code, and we make the compiler happy by wrapping the breaks inside some horror.

> 
> > PerformanceTests/JSAir/payload-gbemu-executeIteration.js:1
> > +"use strict";
> 
> Should we generate a copyright?

Hmmm.  I don't know how to copyright it to.  I think that saying that it's generated code conveys the right message.

> 
> > Source/JavaScriptCore/b3/air/AirDumpAsJS.h:37
> > +// This is a joke. Various operations on Air are interesting from a benchmarking standpoint. We can
> 
> "This is a joke." => "This is used for benchmarking."

Right. :-)

> 
> > Source/WTF/wtf/PrintStream.h:75
> > +    void println(const Types&... values)
> 
> I will actually use this a lot

[Joseph Pecoraro]

Comment on attachment 280737 [details]
almost the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280737&action=review

Neat! I pointed out some places where you could use a few more ES6 features / syntax, since that seemed to be a goal.

> PerformanceTests/JSAir/arg.js:83
> +        switch (role)
> +        {

Weird style.

> PerformanceTests/JSAir/basic_block.js:76
> +            get: function(target, property) {

Style: With ES6 shorthand style you could write these as:

    get(target, property) {
        ...
    },

    set(target, property, value) {
        ...
    }

You use this pattern in other places.

> PerformanceTests/JSAir/code.js:41
> +    addBlock(frequency)
> +    {
> +        if (frequency == null)
> +            frequency = 1;

You use this pattern a bunch. If you want to use more ES6 features you could test default parameter values:

    addBlock(frequency=1)

> PerformanceTests/JSAir/code.js:49
> +        let result = addIndexed(this._stackSlots, StackSlot, byteSize, kind);
> +        return result;

Drop the temporary variable and make this a tail call?

> PerformanceTests/JSAir/code.js:55
> +        let result = addIndexed(this["_" + symbolName(type).toLowerCase() + "Tmps"], Tmp, type);
> +        return result;

Drop the temporary variable and make this a tail call?

> PerformanceTests/JSAir/code.js:96
> +        if (this._frameSize)
> +            result += "Frame size: " + this._frameSize + "\n";
> +        if (this._callArgAreaSize)
> +            result += "Call arg area size: " + this._callArgAreaSize + "\n";

If you want to test more ES6 features, you could use template strings and interpolation in a lot of these toString methods and other places:

    if (this._frameSize)
        result += `Frame size: ${this._frameSize}\n`;
    if (this._callArgAreaSize)
        result += `Call arg area size: ${this._callArgAreaSize}\n`;

> PerformanceTests/JSAir/custom.js:56
> +        for (let i = 0; i < inst.args.length; ++i) {
> +            inst.visitArg(
> +                i, func, inst.patchArgData[i].role, inst.patchArgData[i].type,
> +                inst.patchArgData[i].width);
> +        }

You could make use of ES6 destructuring here:

    let {role, type, width} = inst.patchArgData[i];
    inst.visitArg(i, func, role, type, width);

> PerformanceTests/JSAir/inst.js:130
> +        return "" + symbolName(this._opcode) + " " + this._args.join(", ");

The "" + at the start seems unnecessary. symbolName appears to always return a string.

> PerformanceTests/JSAir/opcode.js:143
> +    case Nop:
> +        break;
> +        break;

A lot of these seem to have duplicate breaks. Might be worth cleaning up in the generator, but ultimately harmless.

> PerformanceTests/JSAir/test.html:28
> +        document.getElementById("result-summary").innerHTML = "That took " + result + " ms.";

You could use `textContent` or `innerText` instead of `innerHTML`:
<http://caniuse.com/#search=textContent>

> PerformanceTests/JSAir/util.js:34
> +function addIndexed(list, cons, ...args)
> +{
> +    let result = new cons(list.length, ...args);

You have this pattern in a few places. It reminds me of bug 158438.

> PerformanceTests/JSAir/util.js:170
> +else if (preciseTime)

This should probably be "if (this.preciseTime)" otherwise you will get a ReferenceError in environments that do not have `global.performance` and no preciseTime global, which I bet would be the case in node/v8.

> PerformanceTests/JSAir/util.js:173
> +else
> +    currentTime = function() { return 0 + new Date(); };

When I run this in the console I get:

    js> (0 + new Date())
    "0Tue Jun 07 2016 14:34:10 GMT-0700 (PDT)"

You should probably just do:

    Date.now()

Or:

    +new Date

> Source/JavaScriptCore/jsc.cpp:2007
> +    if (nameValue.toWTFString(globalObject->globalExec()) == "SyntaxError"

Awesome!

[Filip Pizlo]

(In reply to comment #19)
> Comment on attachment 280737 [details]
> almost the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280737&action=review
> 
> Neat! I pointed out some places where you could use a few more ES6 features
> / syntax, since that seemed to be a goal.
> 
> > PerformanceTests/JSAir/arg.js:83
> > +        switch (role)
> > +        {
> 
> Weird style.

Ooops!  Fixed.

> 
> > PerformanceTests/JSAir/basic_block.js:76
> > +            get: function(target, property) {
> 
> Style: With ES6 shorthand style you could write these as:
> 
>     get(target, property) {
>         ...
>     },
> 
>     set(target, property, value) {
>         ...
>     }
> 
> You use this pattern in other places.

Fixed!

> 
> > PerformanceTests/JSAir/code.js:41
> > +    addBlock(frequency)
> > +    {
> > +        if (frequency == null)
> > +            frequency = 1;
> 
> You use this pattern a bunch. If you want to use more ES6 features you could
> test default parameter values:
> 
>     addBlock(frequency=1)

Filed: https://bugs.webkit.org/show_bug.cgi?id=158497

> 
> > PerformanceTests/JSAir/code.js:49
> > +        let result = addIndexed(this._stackSlots, StackSlot, byteSize, kind);
> > +        return result;
> 
> Drop the temporary variable and make this a tail call?

I wanted to see where I got into addIndexed() from, so I wrote it this way to avoid the tail call.  I like that I can control PTC.

> 
> > PerformanceTests/JSAir/code.js:55
> > +        let result = addIndexed(this["_" + symbolName(type).toLowerCase() + "Tmps"], Tmp, type);
> > +        return result;
> 
> Drop the temporary variable and make this a tail call?

Ditto.

> 
> > PerformanceTests/JSAir/code.js:96
> > +        if (this._frameSize)
> > +            result += "Frame size: " + this._frameSize + "\n";
> > +        if (this._callArgAreaSize)
> > +            result += "Call arg area size: " + this._callArgAreaSize + "\n";
> 
> If you want to test more ES6 features, you could use template strings and
> interpolation in a lot of these toString methods and other places:
> 
>     if (this._frameSize)
>         result += `Frame size: ${this._frameSize}\n`;
>     if (this._callArgAreaSize)
>         result += `Call arg area size: ${this._callArgAreaSize}\n`;

Filed: https://bugs.webkit.org/show_bug.cgi?id=158497

> 
> > PerformanceTests/JSAir/custom.js:56
> > +        for (let i = 0; i < inst.args.length; ++i) {
> > +            inst.visitArg(
> > +                i, func, inst.patchArgData[i].role, inst.patchArgData[i].type,
> > +                inst.patchArgData[i].width);
> > +        }
> 
> You could make use of ES6 destructuring here:
> 
>     let {role, type, width} = inst.patchArgData[i];
>     inst.visitArg(i, func, role, type, width);

Ditto.

> 
> > PerformanceTests/JSAir/inst.js:130
> > +        return "" + symbolName(this._opcode) + " " + this._args.join(", ");
> 
> The "" + at the start seems unnecessary. symbolName appears to always return
> a string.

Yeah, this is an old style of mine.  I guess that this should use interpolation.

> 
> > PerformanceTests/JSAir/opcode.js:143
> > +    case Nop:
> > +        break;
> > +        break;
> 
> A lot of these seem to have duplicate breaks. Might be worth cleaning up in
> the generator, but ultimately harmless.

It's hard to avoid this given the complex logic of the generator.  When it generates C++, it makes similar mistakes, and we have crazy macros for disabling compiler warnings for this purpose.

> 
> > PerformanceTests/JSAir/test.html:28
> > +        document.getElementById("result-summary").innerHTML = "That took " + result + " ms.";
> 
> You could use `textContent` or `innerText` instead of `innerHTML`:
> <http://caniuse.com/#search=textContent>

I changed to use innerText, but how is that different?

> 
> > PerformanceTests/JSAir/util.js:34
> > +function addIndexed(list, cons, ...args)
> > +{
> > +    let result = new cons(list.length, ...args);
> 
> You have this pattern in a few places. It reminds me of bug 158438.

Should I avoid it?

> 
> > PerformanceTests/JSAir/util.js:170
> > +else if (preciseTime)
> 
> This should probably be "if (this.preciseTime)" otherwise you will get a
> ReferenceError in environments that do not have `global.performance` and no
> preciseTime global, which I bet would be the case in node/v8.

Oh right, I'm in strict mode.  Fixed.

> 
> > PerformanceTests/JSAir/util.js:173
> > +else
> > +    currentTime = function() { return 0 + new Date(); };
> 
> When I run this in the console I get:
> 
>     js> (0 + new Date())
>     "0Tue Jun 07 2016 14:34:10 GMT-0700 (PDT)"
> 
> You should probably just do:
> 
>     Date.now()
> 
> Or:
> 
>     +new Date

Fixed!

> 
> > Source/JavaScriptCore/jsc.cpp:2007
> > +    if (nameValue.toWTFString(globalObject->globalExec()) == "SyntaxError"
> 
> Awesome!

[Joseph Pecoraro]

> > > PerformanceTests/JSAir/util.js:34
> > > +function addIndexed(list, cons, ...args)
> > > +{
> > > +    let result = new cons(list.length, ...args);
> > 
> > You have this pattern in a few places. It reminds me of bug 158438.
> 
> Should I avoid it?

Keep it! I was hoping to give encouragement for someone to peek at the bug, which I'm guessing this, and a few other places in this code that use this pattern, may be hitting. =)

[Filip Pizlo]

(In reply to comment #18)
> (In reply to comment #14)
> > Comment on attachment 280735 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280735&action=review
> > 
> > I'm not going to read every line, but LGTM
> > 
> > > PerformanceTests/ChangeLog:21
> > > +        - JSAir's largest hot function, which is anonymous, so we call it by its hash (ACLj8C).
> > 
> > Maybe it's worth giving the anonymous function a name?
> 
> I'll try to check this.  It's probably an arrow function.

Meh.  It's a really stupid closure:

function (arg, role, type, width) => {
            return arg.forEach(thing, role, type, width, func);
        }

It ends up being huge because we inline arg.forEach().

> 
> > 
> > > PerformanceTests/JSAir/benchmark.js:1
> > > +"use strict";
> > 
> > I think it's nicer for this to go after copyright. It's less likely to be
> > glanced over
> > Same goes for in all other files.
> 
> I wasn't sure if that worked or not.  I'll fix this.

Done.

> 
> > 
> > > PerformanceTests/JSAir/opcode.js:736
> > > +        break;
> > > +        break;
> > 
> > Many duplicate breaks
> 
> Yeah, this is generated code.  The code generator isn't that smart.  It does
> the same thing when generating C++ code, and we make the compiler happy by
> wrapping the breaks inside some horror.
> 
> > 
> > > PerformanceTests/JSAir/payload-gbemu-executeIteration.js:1
> > > +"use strict";
> > 
> > Should we generate a copyright?
> 
> Hmmm.  I don't know how to copyright it to.  I think that saying that it's
> generated code conveys the right message.
> 
> > 
> > > Source/JavaScriptCore/b3/air/AirDumpAsJS.h:37
> > > +// This is a joke. Various operations on Air are interesting from a benchmarking standpoint. We can
> > 
> > "This is a joke." => "This is used for benchmarking."
> 
> Right. :-)

Fixed.

> 
> > 
> > > Source/WTF/wtf/PrintStream.h:75
> > > +    void println(const Types&... values)
> > 
> > I will actually use this a lot

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/201783

[Csaba Osztrogonác]

(In reply to comment #23)
> Landed in http://trac.webkit.org/changeset/201783

The new JSAir test made performance bots fail:

Running JSAir/test.html (76 of 152)
ERROR: layer at (0,0) size 800x600

[Filip Pizlo]

(In reply to comment #24)
> (In reply to comment #23)
> > Landed in http://trac.webkit.org/changeset/201783
> 
> The new JSAir test made performance bots fail:
> 
> Running JSAir/test.html (76 of 152)
> ERROR: layer at (0,0) size 800x600

Why did the performance bots run this?  Are they just looking for every .html file in every directory?

[Csaba Osztrogonác]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > Landed in http://trac.webkit.org/changeset/201783
> > 
> > The new JSAir test made performance bots fail:
> > 
> > Running JSAir/test.html (76 of 152)
> > ERROR: layer at (0,0) size 800x600
> 
> Why did the performance bots run this?  Are they just looking for every
> .html file in every directory?

Yes, it runs all tests in PerformanceTests directory, except ones in Skipped file.

[Filip Pizlo]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #23)
> > > > Landed in http://trac.webkit.org/changeset/201783
> > > 
> > > The new JSAir test made performance bots fail:
> > > 
> > > Running JSAir/test.html (76 of 152)
> > > ERROR: layer at (0,0) size 800x600
> > 
> > Why did the performance bots run this?  Are they just looking for every
> > .html file in every directory?
> 
> Yes, it runs all tests in PerformanceTests directory, except ones in Skipped
> file.

Should be fixed: http://trac.webkit.org/changeset/201878