Bug 182165 – JavaScriptCore builtins should be partially minified in Release builds not Debug builds

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

JavaScriptCore builtins should be partially minified in Release builds not Debug builds

Added with:
<https://webkit.org/b/152290> Builtin source should be minified more
http://trac.webkit.org/changeset/194144

> diff --git a/Source/JavaScriptCore/Scripts/builtins/builtins_model.py b/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> old mode 100644
> new mode 100755
> index d765eca865e..cdd5f900354
> --- a/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> +++ b/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> @@ -100,6 +103,11 @@ class BuiltinFunction:
>      @staticmethod
>      def fromString(function_string):
>          function_source = multilineCommentRegExp.sub("", function_string)
> +        if os.getenv("CONFIGURATION", "Debug").startswith("Debug"):
> +            function_source = lineWithOnlySingleLineCommentRegExp.sub("", function_source)
> +            function_source = lineWithTrailingSingleLineCommentRegExp.sub("\n", function_source)
> +            function_source = multipleEmptyLinesRegExp.sub("\n", function_source)

Should have been `if os.getenv("CONFIGURATION", "Debug").startswith("Debug"):` otherwise we are only doing these optimizations for Debug builds.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 332350 [details]
[PATCH] Proposed Fix

Lets see what the bots think.

[Joseph Pecoraro]

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

[EWS Watchlist]

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

[Joseph Pecoraro]

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

[EWS Watchlist]

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.

[Joseph Pecoraro]

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

[Keith Miller]

Looks like you need to rebase.

[Keith Miller]

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!

[Joseph Pecoraro]

Created attachment 332353 [details]
[PATCH] Proposed Fix

Rebaselined.

[EWS Watchlist]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Keith Miller]

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

[WebKit Commit Bot]

Comment on attachment 332354 [details]
[PATCH] Eliminate all leading whitespace version

Clearing flags on attachment: 332354

Committed r227685: <https://trac.webkit.org/changeset/227685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36917860>

[Matt Lewis]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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