perf(rust/renamer): new deconflict strategy by IWANABETHATGUY · Pull Request #1461 · rolldown/rolldown

IWANABETHATGUY · 2024-06-27T16:22:45Z

Description

Closed Need a new strategy for dealing with conflicted identifiers #389

…l-symbol1

chore: 🤖 update snap chore: 🤖 clean chore: 🤖 lint chore: 🤖 revert

chore: 🤖 lint chore: 🤖 clean chore: 🤖 try chore: 🤖 update chore: 🤖 cleran chore: 🤖 clean chore: 🤖 clean

netlify · 2024-06-27T16:24:42Z

✅ Deploy Preview for rolldown-rs canceled.

Name	Link
🔨 Latest commit	`7a70b89`
🔍 Latest deploy log	https://app.netlify.com/sites/rolldown-rs/deploys/667e4a61a389530008c54a20

codecov · 2024-06-27T16:26:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.99%. Comparing base (5df84a8) to head (7a70b89).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1461      +/-   ##
==========================================
+ Coverage   85.98%   85.99%   +0.01%     
==========================================
  Files         152      152              
  Lines        7741     7747       +6     
==========================================
+ Hits         6656     6662       +6     
  Misses       1085     1085

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-06-27T16:30:44Z

Benchmarks Rust

target: main(03548ac)
pr: perf/add-top-level-symbol2(7a70b89)

group                                 pr                                     target
-----                                 --                                     ------
bundle/bundle@rome-ts                 1.00     92.9±0.67ms        ? ?/sec    1.00     93.1±0.75ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap       1.01    111.8±1.80ms        ? ?/sec    1.00    111.0±0.75ms        ? ?/sec
bundle/bundle@threejs                 1.00     28.1±0.81ms        ? ?/sec    1.00     28.0±0.57ms        ? ?/sec
bundle/bundle@threejs-sourcemap       1.00     36.5±0.63ms        ? ?/sec    1.01     36.8±0.86ms        ? ?/sec
bundle/bundle@threejs10x              1.00    301.5±2.43ms        ? ?/sec    1.02    308.3±4.81ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap    1.00    400.4±7.92ms        ? ?/sec    1.03    411.0±8.32ms        ? ?/sec
scan/scan@rome-ts                     1.00     75.9±0.65ms        ? ?/sec    1.00     76.3±0.63ms        ? ?/sec
scan/scan@rome-ts-sourcemap           1.00     76.6±0.66ms        ? ?/sec    1.00     76.9±0.91ms        ? ?/sec
scan/scan@threejs                     1.00     20.9±0.37ms        ? ?/sec    1.00     20.9±0.44ms        ? ?/sec
scan/scan@threejs-sourcemap           1.00     20.7±0.16ms        ? ?/sec    1.01     20.9±0.19ms        ? ?/sec
scan/scan@threejs10x                  1.00    208.3±1.73ms        ? ?/sec    1.01    209.8±2.53ms        ? ?/sec
scan/scan@threejs10x-sourcemap        1.00    208.2±1.66ms        ? ?/sec    1.02    211.7±1.86ms        ? ?/sec

chore: 🤖 update comments chore: 🤖 improve comments

hyfdev · 2024-06-28T05:02:01Z

+              let next_conflict_index = *occ.get() + 1;
+              *occ.get_mut() = next_conflict_index;
+              candidate_name =
+                Cow::Owned(format!("{candidate_name}${next_conflict_index}",).into());


Suggested change

Cow::Owned(format!("{candidate_name}${next_conflict_index}",).into());

Cow::Owned(format!("{original_name}${next_conflict_index}",).into());

hyfdev · 2024-06-28T05:02:23Z

-          count += 1;
-          candidate_name = Cow::Owned(format!("{original_name}${count}").into());
+        loop {
+          match self.used_canonical_names.entry(candidate_name.clone()) {


Suggested change

match self.used_canonical_names.entry(candidate_name.clone()) {

match self.used_canonical_names.entry(original_name.clone()) {

Should you use original_name name here?

We should use candidate name here, you could ref the algorithm https://github.com/rolldown/rolldown/pull/1461/files#diff-c87caec33470b59eac43cd6ac05bef999e6a4fa010e716fd3e546971148f01d2R15,

We need to find a fixed point

If there's is another a, in the current algorithm, will it be renamed to a$1$1$1? If so, I would like to see if we could fix this, it's kind of ugly in the output.

If we don't care the how output looks, we could follow how parcel did which is renaming the vars to a meaningless hashed value. This way it should be more faster.

We can't use original_name here, it will lead a dead loop, you could see the case in https://github.com/rolldown/rolldown/pull/1461/files#diff-c87caec33470b59eac43cd6ac05bef999e6a4fa010e716fd3e546971148f01d2R15

hyfdev · 2024-06-28T05:02:57Z


 //#region shared.js
-const a$2 = 'shared.js';
+const a$1$1 = 'shared.js';


should be a$n rather than a$1$1

Alright, I get you, let me test if performance got regressed

hyfdev

Well done!

IWANABETHATGUY added 8 commits June 27, 2024 21:58

perf: ⚡️ add top level symbol

423a862

chore: 🤖 update snap

1ac0194

chore: 🤖 update snap

feef991

chore: 🤖 update

aad295d

chore: 🤖 remove .rev

9b22fb4

Merge branch 'chore/don-not-rename-from-entry' into perf/add-top-leve…

b160a67

…l-symbol1

chore: 🤖 redundant file

c5b0e24

chore: 🤖 update snap chore: 🤖 clean chore: 🤖 lint chore: 🤖 revert

chore: 🤖 update

b916792

chore: 🤖 lint chore: 🤖 clean chore: 🤖 try chore: 🤖 update chore: 🤖 cleran chore: 🤖 clean chore: 🤖 clean

github-actions Bot added the needs-triage label Jun 27, 2024

IWANABETHATGUY mentioned this pull request Jun 27, 2024

perf: add top level symbol #1460

Closed

IWANABETHATGUY changed the title ~~Perf/add top level symbol2~~ perf: add top level symbol Jun 27, 2024

chore: 🤖 add comments

7376db9

chore: 🤖 update comments chore: 🤖 improve comments

IWANABETHATGUY marked this pull request as ready for review June 27, 2024 16:49

IWANABETHATGUY force-pushed the perf/add-top-level-symbol2 branch from 02bac33 to 7376db9 Compare June 27, 2024 16:49

IWANABETHATGUY requested a review from hyfdev June 28, 2024 02:51

IWANABETHATGUY enabled auto-merge June 28, 2024 02:51

Merge branch 'main' into perf/add-top-level-symbol2

b196580

IWANABETHATGUY mentioned this pull request Jun 28, 2024

Question: should we add a conflict heavy benchmark case to avoid regressing Renamer ? #1466

Closed

hyfdev reviewed Jun 28, 2024

View reviewed changes

IWANABETHATGUY added 2 commits June 28, 2024 13:28

chore: 🤖 update strategy

1cd69d8

chore: 🤖 update comments

7a70b89

hyfdev changed the title ~~perf: add top level symbol~~ perf(rust/renamer): new deconflict strategy Jun 28, 2024

hyfdev disabled auto-merge June 28, 2024 05:36

hyfdev enabled auto-merge June 28, 2024 05:36

hyfdev approved these changes Jun 28, 2024

View reviewed changes

hyfdev added this pull request to the merge queue Jun 28, 2024

Merged via the queue into main with commit fa866eb Jun 28, 2024

hyfdev deleted the perf/add-top-level-symbol2 branch June 28, 2024 05:48

	Cow::Owned(format!("{candidate_name}${next_conflict_index}",).into());
	Cow::Owned(format!("{original_name}${next_conflict_index}",).into());

	match self.used_canonical_names.entry(candidate_name.clone()) {
	match self.used_canonical_names.entry(original_name.clone()) {

Uh oh!

Conversation

IWANABETHATGUY commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

netlify Bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs canceled.

Uh oh!

codecov Bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks Rust

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyfdev Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IWANABETHATGUY Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyfdev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IWANABETHATGUY commented Jun 27, 2024 •

edited

Loading

netlify Bot commented Jun 27, 2024 •

edited

Loading

codecov Bot commented Jun 27, 2024 •

edited

Loading

github-actions Bot commented Jun 27, 2024 •

edited

Loading

hyfdev Jun 28, 2024 •

edited

Loading

IWANABETHATGUY Jun 28, 2024 •

edited

Loading