Skip to content

fix(splitting): indirect external symbol#3405

Merged
underfin merged 3 commits into
mainfrom
fix-indirect-external-symbol-spltting-case
Jan 27, 2025
Merged

fix(splitting): indirect external symbol#3405
underfin merged 3 commits into
mainfrom
fix-indirect-external-symbol-spltting-case

Conversation

@underfin

@underfin underfin commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

Description

fix nuxt + rolldown crashes at crates/rolldown_common/src/types/symbol_ref_db.rs, ref https://github.com/vitejs/vite-ecosystem-ci/blob/rolldown-vite/README-temp.md#nuxt.

The issue happened because we merge all external symbol to one, but not consider the exported external symbol is could split, the referenced external symbol is used other chunks exported symbols, the symbol is not deconflicted at importer chunk.

The pr intend to avoid connecting cross module symbols from external modules, it will ensure correctness for calculate cross module symbols. Here is a esbuild repl. It meaning we only merge import symbols from external modules, if it is exported, it can't be merged.

@underfin underfin changed the title fix(spltting): indirect external symbol fix(splitting): indirect external symbol Jan 23, 2025
@github-actions

github-actions Bot commented Jan 23, 2025

Copy link
Copy Markdown
Contributor

Benchmarks Rust

  • target: main(5deb279)
  • pr: fix-indirect-external-symbol-spltting-case(10a99b3)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.00     79.2±1.38ms        ? ?/sec    1.08     85.1±2.33ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.00     99.4±1.14ms        ? ?/sec    1.05    104.2±2.56ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00    111.4±1.87ms        ? ?/sec    1.03    114.4±1.82ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.00     85.4±1.12ms        ? ?/sec    1.06     90.8±1.55ms        ? ?/sec
bundle/bundle@rome-ts                                               1.00    127.9±1.59ms        ? ?/sec    1.03    132.1±2.18ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.04    234.8±5.64ms        ? ?/sec    1.00    225.5±3.58ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.00    271.7±6.18ms        ? ?/sec    1.00   272.7±22.21ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.00    140.0±1.33ms        ? ?/sec    1.05    146.5±2.70ms        ? ?/sec
bundle/bundle@threejs                                               1.00     43.0±2.09ms        ? ?/sec    1.03     44.2±2.84ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.00     98.4±1.36ms        ? ?/sec    1.01     99.7±0.96ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.01    118.6±1.44ms        ? ?/sec    1.00    117.1±1.13ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.00     53.0±0.89ms        ? ?/sec    1.02     54.2±0.71ms        ? ?/sec
bundle/bundle@threejs10x                                            1.01    457.2±9.71ms        ? ?/sec    1.00    454.1±5.26ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.00  1176.3±17.99ms        ? ?/sec    1.00  1174.2±15.60ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.01  1341.8±19.08ms        ? ?/sec    1.00  1334.7±16.14ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00   535.6±13.36ms        ? ?/sec    1.00    535.6±5.83ms        ? ?/sec
remapping/remapping                                                 1.02     27.3±0.24ms        ? ?/sec    1.00     26.8±0.68ms        ? ?/sec
remapping/render-chunk-remapping                                    1.01     72.5±6.09ms        ? ?/sec    1.00     71.5±5.70ms        ? ?/sec
scan/scan@rome-ts                                                   1.02    101.6±1.81ms        ? ?/sec    1.00     99.4±1.33ms        ? ?/sec
scan/scan@threejs                                                   1.02     32.8±1.90ms        ? ?/sec    1.00     32.2±0.57ms        ? ?/sec
scan/scan@threejs10x                                                1.01    330.5±4.60ms        ? ?/sec    1.00    326.3±4.69ms        ? ?/sec

@IWANABETHATGUY

IWANABETHATGUY commented Jan 23, 2025

Copy link
Copy Markdown
Member

I will review it later, busy with other work.

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jan 26, 2025
Open
@underfin
underfin added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 89db1f3 Jan 27, 2025
@underfin
underfin deleted the fix-indirect-external-symbol-spltting-case branch January 27, 2025 02:29
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jul 12, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@IWANABETHATGUY IWANABETHATGUY IWANABETHATGUY approved these changes

Assignees

No one assigned

Labels

needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@underfin @IWANABETHATGUY