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

Document the fact that Closure advanced optimizations are disabled #598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrianholovaty
Copy link

Hello — this pull request documents the fact that these benchmarks disable Google Closure Compiler's "advanced optimizations." In my opinion this is important context for interpreting the results.

The specific context I intend to communicate in the pull request is:

  • These benchmarks run with Closure Compiler's advanced optimizations turned off.
  • It's not feasible to turn on the advanced optimizations for these benchmarks, because the advanced optimizations require you to structure code in a certain way. These benchmarks run minification on third-party JS libraries, with the assumption that the code is "out of our control."

If people are using these benchmarks to decide which minification library to use on their own JS projects, they should be aware of the Closure Compiler advanced optimizations possibility. Otherwise this set of benchmarks is misleading — or, at the very least, paints an incomplete picture.

I have no affiliation with Google or with the Closure Compiler project, aside from having used it for 10+ years now in my own projects. If you take the time to structure your code to take advantage of the "advanced optimizations," it results in significantly better minification than any other library.

Previously: issue #168 brought up the fact that these benchmarks don't use advanced optimizations, and the issue was closed. If the scope of these benchmarks is to remain on third-party JS libraries, I agree it makes sense not to do an advanced optimizations test — because that would require you to tweak those third-party JS libraries. But I do think you should be transparent about this limitation.

@@ -13,7 +13,7 @@ This project benchmarks the following minifiers:
- [babel-minify](https://github.com/babel/minify/tree/master/packages/babel-minify) <sub>v0.5.2</sub>
- [bun](https://github.com/oven-sh/bun) <sub>v1.1.29</sub>
- [esbuild](https://github.com/evanw/esbuild) <sub>v0.24.0</sub>
- [google-closure-compiler](https://github.com/google/closure-compiler-npm/tree/master/packages/google-closure-compiler) <sub>v20240317.0.0</sub>
- [google-closure-compiler](https://github.com/google/closure-compiler-npm/tree/master/packages/google-closure-compiler) <sub>v20240317.0.0, advanced optimizations disabled</sub>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is generated (between <!-- minifiers:start --> and <!-- minifiers:end -->) so this will get overwritten.

@privatenumber
Copy link
Owner

Thank you for the thoughtful input @adrianholovaty

My goal with this project is to evaluate JS minifiers on equal footing, using minimal/default configurations. I make an exception for Terser and Uglify as the "no compress" option is widely used in practice.

That said, other minifiers also have configurations and optimizations that I'm not utilizing. If we highlight that Google Closure Compiler's advanced optimizations aren't being used, we would also need to point out the other optimizations that aren't leveraged across all the minifiers.

To avoid opening that can of worms, I might prefer to update the Methodology section to clarify that these benchmarks don't demonstrate the full potential of each minifier and that they use default settings and process arbitrary third-party code untailored to any specific optimizations. We can also encourage users to learn more about each minifier to understand the full range of optimizations.

What do you think?

@adrianholovaty
Copy link
Author

@privatenumber Thanks for the response. I definitely understand not wanting to open that can of worms. :) Your idea to update the Methodology section sounds like a positive move, for sure.

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

Successfully merging this pull request may close these issues.

2 participants