Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-add Minification Mapping Input Suggestion #4097

Open
tommywilkinson opened this issue Jun 7, 2023 · 5 comments
Open

Re-add Minification Mapping Input Suggestion #4097

tommywilkinson opened this issue Jun 7, 2023 · 5 comments

Comments

@tommywilkinson
Copy link

Looks like this ability used to be in the compiler, but was removed long ago. A piece of the functionality was re-added for #433, but not the piece allowing for an input mapping to use as a starting point.

It's a very common practice for website to use hashed file names to allow browsers to cache unchanged portions of scripts, especially as a part of a webpack bundle, which allows only certain portions of the bundle to need to get re-downloaded by users in the new version.

However, if you're using Closure Compiler as a minifier (perhaps via the webpack plugin), the instability of the minification will mean that all chunk contents will likely change every build, even if there were no real changes to the contents of some of the chunks.

In an ideal world, we'd be able to give Closure the minification map of the previous build, which it could then use as a "suggestion" for the current mapping. If there's a big enough upside, it can choose to re-map any or all of the identifiers. This means that for cases where there's no upside to re-mapping, Closure could choose stability, perhaps allowing more of those chunks to have unchanged contents.

The main pitch here is to make minification slightly less "chaotic" (in the sense that small changes anywhere within the scope can lead to big output differences everywhere) for cases where there is little-to-no upside.

@L3P3
Copy link

L3P3 commented Jun 7, 2023

Also good for any diffing use, including differential updates.

@nhelfman
Copy link
Contributor

nhelfman commented Dec 26, 2023

@tommywilkinson To expand on the use case described - if one wishes to use Compress Dictionary Transport (CDT)(https://github.com/WICG/compression-dictionary-transport) this is likely a must. The CDT feature enables diff download of JS resources using compression dictionaries. Without it, the typical diff between builds will be too large to be useful with CDT.

Experimenting locally I found that sometimes even small actual difference in origin code results in a size diff of minified code of over 40% due to renaming which is inconsistent between builds.

For example, the following is the same code with different renames:

< var rk = "function" == typeof Object.defineProperties ? Object.defineProperty : function(C, y, a) {
<     if (C == Array.prototype || C == Object.prototype)
<         return C;
<     C[y] = a.value;
<     return C
---
> var Ho = "function" == typeof Object.defineProperties ? Object.defineProperty : function(z, y, a) {
>     if (z == Array.prototype || z == Object.prototype)
>         return z;
>     z[y] = a.value;
>     return z

When using compression dictionary (e.g. brotli) it will result with as a large diff.

What would it take to get this prioritized?

@niloc132
Copy link
Contributor

niloc132 commented Dec 26, 2023

It looks like the feature might still be present, just not plumbed to the command line?

private String variableMapInputFile = "";
/**
* File containing the serialized version of the variable renaming map produced by a previous
* compilation
*/
@CanIgnoreReturnValue
public CommandLineConfig setVariableMapInputFile(String variableMapInputFile) {
this.variableMapInputFile = variableMapInputFile;
return this;
}

Have you tried generating the prop/variable maps and then reading them using this to confirm it doesn't work? I recognize that the commit linked at the top claims that it isn't intended for this purpose, but the tests seem to confirm that it works, and there are compiler passes that check for the presence of these input maps and make use of them to keep names consistent:

/**
* Runs through the list of properties and renames as many as possible with
* names from the previous compilation. Also, updates reservedNames with the
* set of reused names.
* @param reservedNames Reserved names to use during renaming.
* @param allProps Properties to rename.
*/
private void reusePropertyNames(Set<String> reservedNames,
Collection<Property> allProps) {
for (Property prop : allProps) {
// Check if this node can reuse a name from a previous compilation - if
// it can set the newName for the property too.
String prevName = prevUsedPropertyMap.lookupNewName(prop.oldName);
if (!generatePseudoNames && prevName != null) {
// We can reuse prevName if it's not reserved.
if (reservedNames.contains(prevName)) {
continue;
}
prop.newName = prevName;
reservedNames.add(prevName);
}
}
}

etc

@nhelfman
Copy link
Contributor

nhelfman commented Dec 27, 2023

This is what I was thinking when I looked at this code earlier.

I plan to try it and will report results here.

@nhelfman
Copy link
Contributor

Update on my testing - when enabling input maps on a local build of the compiler I was able to create a brolti diff of 10% between 2 production versions of our code. This is compared to the 40% diff without it. This means that this feature is very important and will play a significant part in our efforts to roll out Compress Dictionary Transport (CDT)(https://github.com/WICG/compression-dictionary-transport)

I'm working on submitting a PR with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants