⚠️ No Changeset found

Latest commit: 2f1f4eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Binary Sizes

Commit: fa5bb5e

Merging this PR will not alter performance

✅ 218 untouched benchmarks

_{Comparing kdy1/fix-11667-transformer-hotspots (2f1f4eb) with main (0f24ee1)}

PR Review: perf(transformer): remove O(n^2) statement mutation hotspots

Overall this is a solid performance improvement PR. The changes are well-scoped, the algorithmic improvements are correct, and the new unit tests provide good coverage for the refactored logic. A few observations:

Positive

• Correct algorithmic fix: Replacing reverse-iterate-and-insert with single-pass rebuild in StmtInjector is the right approach. The old code was O(n*k) due to repeated Vec::insert calls that shift elements; the new code is O(n+k).
• Good test coverage: The three new unit tests in statement_injector.rs cover single-target before/after ordering, multi-target ordering, and module-item-specific behavior. These directly validate the invariants of the rewrite.
• FxHashSet for membership checks: Switching methods and statics from Vec<Atom> to FxHashSet<Atom> in private_property_in_object.rs is a clear win — these are only used for .contains() lookups, never iterated in order.
• Clean refactor of exit_module_items: The object-rest exit_module_items rewrite from index-mutation loop to for item in original is much more readable and avoids the fragile items.remove(i) / items.insert(i, ...) pattern.

Minor Issues / Suggestions

1. prepend_stmts_to_front with vec![single_stmt]: There are three call sites that pass vec![stmt] (lines 434, 535, 574). For these single-element cases, the function allocates a temporary Vec, then mem::takes the original, allocates a new merged vec, and does two append calls. A simple stmts.insert(0, stmt) would be cheaper for a single element when stmts is small (which it typically is for a function/catch body). Consider either:

   • An overload like prepend_stmt_to_front(stmts: &mut Vec<Stmt>, stmt: Stmt) for the single-element case that just does stmts.insert(0, stmt).
   • Or accepting a single Stmt parameter alongside the Vec variant.

   That said, this is a minor nit — the hot paths (collector.into_stmts() cases) already pass multi-element vectors, and the single-element sites are unlikely to be hot.

2. after vec allocation inside the loop: In both exit_module_items and exit_stmts, a let mut after = Vec::new() is allocated on every iteration of the loop, even for items that have no insertions (the common case). Consider hoisting the after vec outside the loop and using after.clear() to reuse the allocation:

   let mut after = Vec::new();
   for (i, item) in original.into_iter().enumerate() {
       after.clear();
       // ...
       rewritten.extend(after.drain(..));
   }

   This avoids repeated small allocations when multiple items have after-insertions.

3. Code duplication between exit_module_items and exit_stmts: The two methods are nearly identical — they differ only in the wrapper type (ModuleItem::Stmt(...) vs bare Stmt). This isn't new to this PR, but the rewrite makes the parallel structure even more visible. A generic helper or macro could reduce the duplication, though this is a matter of taste and not a blocker.

Correctness

• The pointer-based address matching relies on the original vec being built from node.take() / stmts.take(), which moves elements out. Since the first pass collects addresses before the take, and take() uses mem::take (which replaces with default, not realloc), the addresses collected in pass 1 remain valid for matching in pass 2. This is correct.
• The insertions iterator is consumed in order, matching monotonically increasing indices from the first pass — the peek/next pattern is correct.

No Issues Found

• No security concerns.
• No public API changes.
• Test coverage is appropriate for the changes.

LGTM with the minor suggestions above.