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.
Created attachment 280369 [details] gotta start by dumping Air to a JS data definition
Created attachment 280371 [details] the C++ side may be written Yucky!
Created attachment 280447 [details] moar
Created attachment 280452 [details] so much fun
Created attachment 280456 [details] a bit more
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.
Created attachment 280642 [details] a bit more
Created attachment 280664 [details] getting there
Created attachment 280666 [details] added customs
Created attachment 280730 [details] getting pretty close
Created attachment 280733 [details] almost done
Created attachment 280735 [details] the patch
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.
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
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.
Created attachment 280737 [details] almost the patch Uploading this to check EWS, but I still need to address Saam's comments.
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.
(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
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!
(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!
> > > 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. =)
(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
Landed in http://trac.webkit.org/changeset/201783
(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
(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?
(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.
(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