Bug 152290 - Builtin source should be minified more
Summary: Builtin source should be minified more
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-14 19:56 PST by Joseph Pecoraro
Modified: 2015-12-16 09:31 PST (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (28.59 KB, patch)
2015-12-14 20:45 PST, Joseph Pecoraro
ysuzuki: review+
ysuzuki: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (2.63 KB, patch)
2015-12-15 11:36 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (2.63 KB, patch)
2015-12-15 11:36 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-12-14 19:56:32 PST
* 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" \
Comment 1 Joseph Pecoraro 2015-12-14 20:04:15 PST
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.
Comment 2 Joseph Pecoraro 2015-12-14 20:05:26 PST
(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.
Comment 3 Joseph Pecoraro 2015-12-14 20:40:02 PST
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.
Comment 4 Joseph Pecoraro 2015-12-14 20:45:23 PST
Created attachment 267337 [details]
[PATCH] Proposed Fix
Comment 5 WebKit Commit Bot 2015-12-14 20:46:17 PST
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`)
Comment 6 Yusuke Suzuki 2015-12-15 01:06:10 PST
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.
Comment 7 Joseph Pecoraro 2015-12-15 11:36:09 PST
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.
Comment 8 Joseph Pecoraro 2015-12-15 11:36:22 PST
Created attachment 267383 [details]
[PATCH] Proposed Fix
Comment 9 WebKit Commit Bot 2015-12-16 09:31:17 PST
Comment on attachment 267383 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 267383

Committed r194144: <http://trac.webkit.org/changeset/194144>
Comment 10 WebKit Commit Bot 2015-12-16 09:31:21 PST
All reviewed patches have been landed.  Closing bug.