Bug 182165

Example of the diff of a before/after the few minifications being applied: 534c463 < const int s_arrayPrototypeSomeCodeLength = 540; --- > const int s_arrayPrototypeSomeCodeLength = 460; 539,554c468,480 < " \"use strict\";\n" \ < "\n" \ < " var array = @toObject(this, \"Array.prototype.some requires that |this| not be null or undefined\");\n" \ < " var length = @toLength(array.length);\n" \ < "\n" \ < " if (typeof callback !== \"function\")\n" \ < " @throwTypeError(\"Array.prototype.some callback must be a function\");\n" \ < " \n" \ < " var thisArg = @argument(1);\n" \ < " for (var i = 0; i < length; i++) {\n" \ < " if (!(i in array))\n" \ < " continue;\n" \ < " if (callback.@call(thisArg, array[i], i, array))\n" \ < " return true;\n" \ < " }\n" \ < " return false;\n" \ --- > "\"use strict\";\n" \ > "var array = @toObject(this, \"Array.prototype.some requires that |this| not be null or undefined\");\n" \ > "var length = @toLength(array.length);\n" \ > "if (typeof callback !== \"function\")\n" \ > " @throwTypeError(\"Array.prototype.some callback must be a function\");\n" \ > "var thisArg = @argument(1);\n" \ > "for (var i = 0; i < length; i++) {\n" \ > " if (!(i in array))\n" \ > " continue;\n" \ > " if (callback.@call(thisArg, array[i], i, array))\n" \ > " return true;\n" \ > "}\n" \ > "return false;\n" \ Lets see what tests will be affected.

Created attachment 332350 [details] [PATCH] Proposed Fix Lets see what the bots think.

Created attachment 332351 [details] [BEFORE] JSCBuiltins.cpp

This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)

Created attachment 332352 [details] [AFTER] JSCBuiltins.cpp

Attachment 332350 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_model.py:130: expected 1 blank line, found 0 [pep8/E301] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

This would also minify WebCore's many built-ins, but those are spread across many files.

Looks like you need to rebase.

Comment on attachment 332350 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=332350&action=review r=me. I always wished we had a static UnlinkedBytecode for builtin js functions... that seems hard for non-webcore though. Also, why not just remove all extraneous whitespace? I doubt it would matter much in practice, though. > Source/JavaScriptCore/Scripts/builtins/builtins_model.py:129 > - if os.getenv("CONFIGURATION", "Debug").startswith("Debug"): > + if not os.getenv("CONFIGURATION", "Debug").startswith("Debug"): Awesome!

Created attachment 332353 [details] [PATCH] Proposed Fix Rebaselined.

Attachment 332353 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_model.py:130: expected 1 blank line, found 0 [pep8/E301] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Keith Miller from comment #9) > Comment on attachment 332350 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332350&action=review > > r=me. I always wished we had a static UnlinkedBytecode for builtin js > functions... that seems hard for non-webcore though. Also, why not just > remove all extraneous whitespace? I doubt it would matter much in practice, > though. I just wanted a cheap solution to eliminate a lot of whitespace. I can entirely eliminate it in Release builds. We could run the jsmin minifier eventually, but a concern there is our jsmin not knowing about new JavaScript syntax could choke and I didn't want something like that to block JavaScriptCore developers from building.

In fact that is what I'll do. I'll eliminate all leading whitespace if the bots seem good with it, since thats an even easier patch. I'll put it up in parallel.

Created attachment 332354 [details] [PATCH] Eliminate all leading whitespace version

(In reply to Joseph Pecoraro from comment #12) > (In reply to Keith Miller from comment #9) > > Comment on attachment 332350 [details] > > [PATCH] Proposed Fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=332350&action=review > > > > r=me. I always wished we had a static UnlinkedBytecode for builtin js > > functions... that seems hard for non-webcore though. Also, why not just > > remove all extraneous whitespace? I doubt it would matter much in practice, > > though. > > I just wanted a cheap solution to eliminate a lot of whitespace. I can > entirely eliminate it in Release builds. > > We could run the jsmin minifier eventually, but a concern there is our jsmin > not knowing about new JavaScript syntax could choke and I didn't want > something like that to block JavaScriptCore developers from building. Sure, I think the conversion to unlinked bytecode would solve that but minifying seems more expedient -- and less of a PITA for the build system --.

Comment on attachment 332354 [details] [PATCH] Eliminate all leading whitespace version Clearing flags on attachment: 332354 Committed r227685: <https://trac.webkit.org/changeset/227685>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36917860>

This caused the builtins-generator-tests to fail. https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/2977 https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/2977/steps/builtins-generator-tests/logs/stdio

(In reply to Matt Lewis from comment #19) > This caused the builtins-generator-tests to fail. > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/2977 > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/2977/steps/builtins- > generator-tests/logs/stdio These just need to be rebaselined now that our default behavior is to not minify (it was accidentally defaulting to minify before. I'll rebasline them.

> These just need to be rebaselined now that our default behavior is to not > minify (it was accidentally defaulting to minify before. I'll rebasline them. <https://trac.webkit.org/r227693>