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

feat(mangler): mangle private class names #9279

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Feb 21, 2025

$ cat test.js
class Foo {
  constructor(k) {
    this.#kasd = k;
  }

  getK() {
    return this.#kasd;
  }
}
class Foo2 {
  constructor(k) {
    this.#kasd = k;
  }

  getK() {
    return this.#kasd;
  }
}

export { Foo, Foo2 };
$ cargo run -p oxc_minifier --example minifier -- --mangle
   Compiling oxc_mangler v0.51.0 (/Users/cameron/github/Boshen/oxc/crates/oxc_mangler)
   Compiling oxc_minifier v0.51.0 (/Users/cameron/github/Boshen/oxc/crates/oxc_minifier)
    Finished `dev` profile [unoptimized] target(s) in 0.96s
     Running `target/debug/examples/minifier --mangle`
class Foo {
        constructor(e) {
                this.#e = e;
        }
        getK() {
                return this.#e;
        }
}
class Foo2 {
        constructor(e) {
                this.#e = e;
        }
        getK() {
                return this.#e;
        }
}
export { Foo, Foo2 };

No diff on cargo misize. looks like non of the test suite has classes with private class members

Copy link
Contributor Author

camc314 commented Feb 21, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camc314 camc314 marked this pull request as ready for review February 21, 2025 16:19
@github-actions github-actions bot added A-minifier Area - Minifier C-enhancement Category - New feature or request labels Feb 21, 2025
@camc314 camc314 force-pushed the c/02-21-feat_mangler_mangle_private_class_names branch from bb4d1c9 to abf1825 Compare February 21, 2025 16:24
@camc314 camc314 force-pushed the c/02-21-refactor_mangler_move_base54_into_seperate_mod branch from 10699ae to abe1153 Compare February 21, 2025 16:24
@camc314 camc314 force-pushed the c/02-21-feat_mangler_mangle_private_class_names branch from abf1825 to 1770de5 Compare February 21, 2025 16:24
Copy link

codspeed-hq bot commented Feb 21, 2025

CodSpeed Performance Report

Merging #9279 will degrade performances by 88.4%

Comparing c/02-21-feat_mangler_mangle_private_class_names (9106d71) with main (4ad328b)

Summary

❌ 3 regressions
✅ 30 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
mangler[antd.js] 16.2 ms 139.3 ms -88.4%
mangler[react.development.js] 299.4 µs 1,766.4 µs -83.05%
mangler[typescript.js] 39.8 ms 280.4 ms -85.79%

Base automatically changed from c/02-21-refactor_mangler_move_base54_into_seperate_mod to main February 22, 2025 05:58
@camc314 camc314 force-pushed the c/02-21-feat_mangler_mangle_private_class_names branch 2 times, most recently from 9bad165 to e723e14 Compare February 22, 2025 17:20
@Boshen
Copy link
Member

Boshen commented Feb 23, 2025

This adds an additional AST pass, but how come it's not shown on the benchmark 🤔 ?

@camc314
Copy link
Contributor Author

camc314 commented Feb 23, 2025

so i implemented this inside:

pub fn build<'a>(self, allocator: &'a Allocator, program: &mut Program<'a>) -> MinifierReturn {

minifier::build. but that fn doesn't run when doing the benches. we run the mangler/minifier explicitly.

i think i could either:

  1. move this directly into the mangler
  2. move this as a minifier pass
    probably 1 is better?

@Boshen
Copy link
Member

Boshen commented Feb 23, 2025

  • move this directly into the mangler

Yeah this seems better. We can also add a flag (turned off by default) to enable this.

@camc314 camc314 force-pushed the c/02-21-feat_mangler_mangle_private_class_names branch from e723e14 to 71b8928 Compare February 24, 2025 11:36
Comment on lines 61 to 69
runner.run(|| {
let _ = Mangler::new().build_with_semantic(semantic, &program);
let _ = Mangler::new()
.with_options(MangleOptions {
debug: false,
top_level: true,
mangle_private_class_names: true,
})
.build(&allocator, &mut program);
});
Copy link
Contributor Author

@camc314 camc314 Feb 24, 2025

Choose a reason for hiding this comment

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

this will regress perf a lot as we are now building semantic inside mangler (inside runner.run, rather than in the prior phase)

I had to make this change because semantic requires a reference to the Program, but we need to pass a mutable reference into here for the private class members to mutate the AST.

i'm open to other ideas:

  • we could have dedicted methods for mangling symbols vs private class members, and a .finish returning the symbol table
  • something else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier C-enhancement Category - New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants