Skip to content

fix(swc): Use fair/correct option #664

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

Closed
wants to merge 6 commits into from
Closed

Conversation

kdy1
Copy link

@kdy1 kdy1 commented Mar 14, 2025

SWC has size.sh for printing the size of each file in this benchmark, but the result is very different. Does creating a PR will automatically run benchmarks?

This is the output of size.sh

File: ./benches/full/antd.js
  456855
File: ./benches/full/d3.js
   86899
File: ./benches/full/echarts.js
  320588
File: ./benches/full/jquery.js
   30878
File: ./benches/full/lodash.js
   25094
File: ./benches/full/moment.js
   18589
File: ./benches/full/react.js
    8194
File: ./benches/full/terser.js
  122634
File: ./benches/full/three.js
  158097
File: ./benches/full/typescript.js
  857900
File: ./benches/full/victory.js
  157199
File: ./benches/full/vue.js
   42651

I removed

    Finished `release` profile [optimized + debuginfo] target(s) in 0.14s
     Running `/Users/kdy1/projects/s/minifier/target/release/examples/minifier ./benches/full/echarts.js`

for brevity

@kdy1 kdy1 changed the title fix(swc): Use correct option fix(swc): Use fair/correct option Mar 14, 2025
@privatenumber
Copy link
Owner

It should, but due to a GitHub security limitation (preventing modifications to another repo from a different repo's GitHub Actions), it doesn't for external contributors.

However, I ran it for you here: #666

You can find the raw stats in the data.json file.

I'll give this more thought. I previously rejected config changes from:

But I've previously made exceptions for Terser and Uglify since their compression modes produce significantly different results.

The key issue is that there are endless configurations optimized for different inputs and we can only bench/compare so much. That said, most users just stick with the default settings, and I want these benchmarks to reflect the typical out-of-the-box experience.

@kdy1
Copy link
Author

kdy1 commented Mar 14, 2025

I'll investigate the reason for the difference. Actually, the minifier is what I expected to be the default or the out-of-the-box thing for the SWC minifier, so there's a bug somewhere in the minify() of @swc/core.

@mischnic
Copy link
Contributor

I understand that you want to prevent the minifiers to game this benchmark by fine tuning the configs too much.

But you should definitely make sure that every minifier removes unused top-level declarations, and also removes comments. Otherwise it's either incorrectly configured or there is a bug in the minifier (as kdy was saying).

@privatenumber
Copy link
Owner

Disabling comments is definitely fine.

Looking into the top-level declarations one. Some minifiers like esbuild simply doesn't support that: evanw/esbuild#3570

kdy1 added a commit to swc-project/swc that referenced this pull request Mar 17, 2025
**Description:**

I made a mistake. This PR also adds a size test for the `swc` crate, to ensure that the issue described in privatenumber/minification-benchmarks#664 is not our fault.
@kdy1
Copy link
Author

kdy1 commented Mar 17, 2025

Closing to cleanup

@kdy1 kdy1 closed this Mar 17, 2025
@kdy1 kdy1 deleted the same-option branch March 17, 2025 16:43
@privatenumber
Copy link
Owner

@kdy1
Copy link
Author

kdy1 commented Mar 18, 2025

Things I found:

fn gzip_size(data: &[u8]) -> usize {
    use flate2::{write::GzEncoder, Compression};
	use std::io::Write;

    let mut encoder = GzEncoder::new(Vec::new(), Compression::best());
    encoder.write_all(data).unwrap();
    encoder.finish().unwrap().len()
}

@kdy1
Copy link
Author

kdy1 commented Mar 18, 2025

Guessing from the API of https://docs.rs/flate2/latest/flate2/write/index.html, I think zlib and gzip format is a different?
There are both GzEncoder and ZlibEncoder.

As a side note, GzEncoder matches the size of gzip -c -9

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.

3 participants