fix(mangler): use `retain` instead of `truncate` to remove empty frequency slots by Dunqing · Pull Request #18225 · oxc-project/oxc

Dunqing · 2026-01-19T11:02:14Z

#18183 introduced a bug when removing empty frequency slots. The code used truncate after sorting:

frequencies.sort_unstable_by_key(|x| std::cmp::Reverse(x.frequency));
if let Some(idx) = frequencies.iter().position(|x| x.symbol_ids.is_empty()) {
    frequencies.truncate(idx);
}

The problem is that sort_unstable_by_key doesn't guarantee order for items with equal keys. When multiple slots have frequency: 0, some have symbols (that need renaming but have no references), and some are empty. Since the sort order is unstable, an empty slot could appear before a non-empty slot, and truncate would incorrectly remove valid slots.

This fix uses retain instead, and moves it before the sort for better efficiency (sorting fewer elements):

frequencies.retain(|x| !x.symbol_ids.is_empty());
frequencies.sort_unstable_by_key(|x| std::cmp::Reverse(x.frequency));

Note for reviewer: I've locally tested in the monitor-oxc; there is no naming collision after this.

Dunqing · 2026-01-19T11:02:32Z

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.

codspeed-hq · 2026-01-19T11:06:45Z

Merging this PR will not alter performance

✅ 38 untouched benchmarks
⏩ 7 skipped benchmarks¹

_{Comparing 01-19-fix_mangler_use_retain_instead_of_truncate_to_remove_empty_frequency_slots (7d9fc4f) with main (8fe1e8e)²}

7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
No successful run was found on main (9a5e3e2) during the generation of this report, so 8fe1e8e was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

Copilot

Pull request overview

This PR fixes a bug in the mangler where empty frequency slots were incorrectly removed using truncate after an unstable sort. The issue was that sort_unstable_by_key doesn't guarantee order for items with equal keys (like frequency=0), so empty slots could appear before non-empty slots containing symbols that need renaming, causing those valid slots to be incorrectly truncated.

Changes:

Replaced truncate logic with retain to filter out empty slots
Moved the filtering before sorting for better efficiency
Updated snapshot test to reflect correct parameter renaming behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
crates/oxc_mangler/src/lib.rs	Replaced `truncate` with `retain` and moved filtering before sort to fix bug and improve performance
crates/oxc_minifier/tests/mangler/snapshots/mangler.snap	Updated snapshot showing parameter `a` now correctly renamed to `e`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Dunqing · 2026-01-19T11:12:16Z

I thought frequencies.sort_unstable_by_key(|x| std::cmp::Reverse(x.frequency)); would place all the empty slots at the end, but that is incorrect. x.frequency is 0, which means no references, but it doesn't mean it doesn't have symbols (symbols can be defined without use).

// Initial:
[{ slot: 0, freq: 0, [] }, { slot: 1, freq: 0, [a] }]

// After sort_unstable_by_key - could be EITHER:
[{ slot: 0, freq: 0, [] }, { slot: 1, freq: 0, [a] }]  // same order
// OR
[{ slot: 1, freq: 0, [a] }, { slot: 0, freq: 0, [] }]  // swapped

Dunqing · 2026-01-19T11:13:05Z

And the performance is actually better than truncate.

sapphi-red · 2026-01-19T11:14:24Z

Ah, my bad, I was thinking of frequencies.sort_unstable_by_key(|x| (std::cmp::Reverse(x.frequency), x.symbol_ids.len()));. Did you try this one?

sapphi-red

(the comment above is non-blocking)

overlookmotel · 2026-01-19T11:43:55Z

I'm going to merge this as is because it's a blocker to doing a release. @Dunqing You may want to look into Sapphi's suggestion later.

overlookmotel · 2026-01-19T11:44:18Z

Merge activity

Jan 19, 11:44 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jan 19, 11:44 AM UTC: overlookmotel added this pull request to the Graphite merge queue.
Jan 19, 11:51 AM UTC: Merged by the Graphite merge queue.

…uency slots (#18225) #18183 introduced a bug when removing empty frequency slots. The code used `truncate` after sorting: ```rust frequencies.sort_unstable_by_key(|x| std::cmp::Reverse(x.frequency)); if let Some(idx) = frequencies.iter().position(|x| x.symbol_ids.is_empty()) { frequencies.truncate(idx); } ``` The problem is that `sort_unstable_by_key` doesn't guarantee order for items with equal keys. When multiple slots have `frequency: 0`, some have symbols (that need renaming but have no references), and some are empty. Since the sort order is unstable, an empty slot could appear **before** a non-empty slot, and `truncate` would incorrectly remove valid slots. This fix uses `retain` instead, and moves it before the sort for better efficiency (sorting fewer elements): ```rust frequencies.retain(|x| !x.symbol_ids.is_empty()); frequencies.sort_unstable_by_key(|x| std::cmp::Reverse(x.frequency)); ``` Note for reviewer: I've locally tested in the `monitor-oxc`; there is no naming collision after this.

Dunqing · 2026-01-19T13:29:11Z

Ah, my bad, I was thinking of frequencies.sort_unstable_by_key(|x| (std::cmp::Reverse(x.frequency), x.symbol_ids.len()));. Did you try this one?

I tried it locally, and it fixed the issue as well. Considering that retain makes Mangler 1% faster than truncate, I won't make a change for this.

### 🐛 Bug Fixes - f1e2dc0 semantic: No error in `check_function_redeclaration` for CommonJS files (#18231) (overlookmotel) - 645c3f0 transformer: Use `require` not `import` in CommonJS files (#18226) (overlookmotel) - ee9f6a4 mangler: Use `retain` instead of `truncate` to remove empty frequency slots (#18225) (Dunqing) ### ⚡ Performance - 52073d9 semantic: Use cheaper test for source type (#18235) (overlookmotel) Co-authored-by: overlookmotel <[email protected]>

github-actions bot added A-minifier Area - Minifier C-bug Category - Bug labels Jan 19, 2026

Dunqing requested review from Boshen and sapphi-red January 19, 2026 11:06

Dunqing assigned sapphi-red Jan 19, 2026

Dunqing marked this pull request as ready for review January 19, 2026 11:06

Copilot AI review requested due to automatic review settings January 19, 2026 11:06

Copilot started reviewing on behalf of Dunqing January 19, 2026 11:06 View session

Dunqing force-pushed the 01-19-fix_mangler_use_retain_instead_of_truncate_to_remove_empty_frequency_slots branch from 35c4846 to 7d9fc4f Compare January 19, 2026 11:07

Copilot AI reviewed Jan 19, 2026

View reviewed changes

sapphi-red approved these changes Jan 19, 2026

View reviewed changes

overlookmotel added the 0-merge Merge with Graphite Merge Queue label Jan 19, 2026

graphite-app bot force-pushed the 01-19-fix_mangler_use_retain_instead_of_truncate_to_remove_empty_frequency_slots branch from 7d9fc4f to ee9f6a4 Compare January 19, 2026 11:44

graphite-app bot merged commit ee9f6a4 into main Jan 19, 2026
21 checks passed

graphite-app bot deleted the 01-19-fix_mangler_use_retain_instead_of_truncate_to_remove_empty_frequency_slots branch January 19, 2026 11:51

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jan 19, 2026

Boshen mentioned this pull request Jan 19, 2026

release(crates): oxc v0.110.0 #18240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

fix(mangler): use `retain` instead of `truncate` to remove empty frequency slots#18225

fix(mangler): use `retain` instead of `truncate` to remove empty frequency slots#18225
graphite-app[bot] merged 1 commit intomainfrom
01-19-fix_mangler_use_retain_instead_of_truncate_to_remove_empty_frequency_slots

Dunqing commented Jan 19, 2026 •

edited

Loading

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

codspeed-hq bot commented Jan 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

sapphi-red commented Jan 19, 2026

Uh oh!

sapphi-red left a comment

Uh oh!

overlookmotel commented Jan 19, 2026

Uh oh!

overlookmotel commented Jan 19, 2026 •

edited by graphite-app bot

Loading

Uh oh!

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Comments

Conversation

Dunqing commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dunqing commented Jan 19, 2026

How to use the Graphite Merge Queue

Uh oh!

codspeed-hq bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

sapphi-red commented Jan 19, 2026

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

overlookmotel commented Jan 19, 2026

Uh oh!

overlookmotel commented Jan 19, 2026 • edited by graphite-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Dunqing commented Jan 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dunqing commented Jan 19, 2026 •

edited

Loading

codspeed-hq bot commented Jan 19, 2026 •

edited

Loading

overlookmotel commented Jan 19, 2026 •

edited by graphite-app bot

Loading