Bug 152290 – Builtin source should be minified more

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Builtin source should be minified.

In many cases, the extracted builtin code can be minified more. Here are some existing examples:

1. Whitespace can be stripped.
Primarily leading whitespace and many trailing newlines. This seems to account for sometimes 20% of the size of these builtins.

2. Lines that end up as only comments can be removed entirely. For example:

> const JSC::ConstructAbility s_moduleLoaderObjectNewRegistryEntryCodeConstructAbility = JSC::ConstructAbility::CannotConstruct;
> const int s_moduleLoaderObjectNewRegistryEntryCodeLength = 713;
> const char* s_moduleLoaderObjectNewRegistryEntryCode =
>     "(function (key)\n" \
>     "{\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "\n" \
>     "    \"use strict\";\n" \
>     "\n" \
>     "    return {\n" \
>     "        key: key,\n" \
>     "        state: this.Fetch,\n" \
>     "        metadata: undefined,\n" \
>     "        fetch: undefined,\n" \
>     "        translate: undefined,\n" \
>     "        instantiate: undefined,\n" \
>     "        resolveDependencies: undefined,\n" \
>     "        dependencies: [], //\n" \
>     "        dependenciesMap: undefined,\n" \
>     "        module: undefined, //\n" \
>     "        error: undefined,\n" \
>     "    };\n" \
>     "})\n" \

[Joseph Pecoraro]

Looks like most of this whitespace is added by:
generate_embedded_code_string_section_for_function

It includes at least the leading 4 spaces of each line and the trailing newline at the end.

[Joseph Pecoraro]

(In reply to comment #1)
> Looks like most of this whitespace is added by:
> generate_embedded_code_string_section_for_function

I take that back, that is whitespace outside the source string.

[Joseph Pecoraro]

I'll handle some basic stuff, like stripping the empty comments and empty lines. I think it leads to even more readable code in the results anyways:

BEFORE:

> const int s_moduleLoaderObjectRequestFetchCodeLength = 669;
> const char* s_moduleLoaderObjectRequestFetchCode =
>     "(function (key)\n" \
>     "{\n" \
>     "    //\n" \
>     "\n" \
>     "    \"use strict\";\n" \
>     "\n" \
>     "    var entry = this.ensureRegistered(key);\n" \
>     "    if (entry.state > this.Link) {\n" \
>     "        var deferred = @newPromiseCapability(@InternalPromise);\n" \
>     "        deferred.@reject.@call(undefined, new @TypeError(\"Requested module is already ready to be executed.\"));\n" \
>     "        return deferred.@promise;\n" \
>     "    }\n" \
>     "\n" \
>     "    if (entry.fetch)\n" \
>     "        return entry.fetch;\n" \
>     "\n" \
>     "    var loader = this;\n" \
>     "\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    //\n" \
>     "    var fetchPromise = this.fetch(key).then(function (payload) {\n" \
>     "        loader.setStateToMax(entry, loader.Translate);\n" \
>     "        return payload;\n" \
>     "    });\n" \
>     "    entry.fetch = fetchPromise;\n" \
>     "    return fetchPromise;\n" \
>     "})\n" \
> ;

AFTER:

> const int s_moduleLoaderObjectRequestFetchCodeLength = 615;
> const char* s_moduleLoaderObjectRequestFetchCode =
>     "(function (key)\n" \
>     "{\n" \
>     "    \"use strict\";\n" \
>     "    var entry = this.ensureRegistered(key);\n" \
>     "    if (entry.state > this.Link) {\n" \
>     "        var deferred = @newPromiseCapability(@InternalPromise);\n" \
>     "        deferred.@reject.@call(undefined, new @TypeError(\"Requested module is already ready to be executed.\"));\n" \
>     "        return deferred.@promise;\n" \
>     "    }\n" \
>     "    if (entry.fetch)\n" \
>     "        return entry.fetch;\n" \
>     "    var loader = this;\n" \
>     "    var fetchPromise = this.fetch(key).then(function (payload) {\n" \
>     "        loader.setStateToMax(entry, loader.Translate);\n" \
>     "        return payload;\n" \
>     "    });\n" \
>     "    entry.fetch = fetchPromise;\n" \
>     "    return fetchPromise;\n" \
>     "})\n" \
> ;

I didn't go all out and minify everything though. I think this is a good balance.

[Joseph Pecoraro]

Created attachment 267337 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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

[Yusuke Suzuki]

Comment on attachment 267337 [details]
[PATCH] Proposed Fix

r=me.
And I also suggest to disable this when building JSC in the debug mode.
In the debug build, we can use @assert buildin intrinsic. And it reports a line number in a given function when the assertion fires.
So, to report meaningful line number, disabling this in the debug mode is nice.

[Joseph Pecoraro]

Created attachment 267382 [details]
[PATCH] Proposed Fix

I wasn't sure how to do this reliably check Debug vs. Non-Debug across all platforms, so I default to not doing the minifying and minify only in builds where the CONFIGURATION environment variable does not start with "Debug". So Mac Release/Production builds would minify.

[Joseph Pecoraro]

Created attachment 267383 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 267383 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 267383

Committed r194144: <http://trac.webkit.org/changeset/194144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.