Skip to content

feat(rust/rolldown): support inject imports#1933

Merged
hyf0 merged 2 commits intomainfrom
inject-plugin
Aug 10, 2024
Merged

feat(rust/rolldown): support inject imports#1933
hyf0 merged 2 commits intomainfrom
inject-plugin

Conversation

@Boshen
Copy link
Copy Markdown
Member

@Boshen Boshen commented Aug 10, 2024

This PR adds the missing pieces for inject plugin.

@Boshen Boshen requested a review from hyf0 August 10, 2024 15:29
@Boshen Boshen removed the request for review from hyf0 August 10, 2024 15:29
@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 10, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 8d3da5c
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66b7a10be7e34c0008667042

Comment thread crates/rolldown/src/utils/pre_process_ecma_ast.rs
Comment thread crates/rolldown/src/utils/pre_process_ecma_ast.rs Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 10, 2024

Benchmarks Rust

group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.00     52.2±0.61ms        ? ?/sec    1.02     53.1±2.50ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.00     75.2±0.84ms        ? ?/sec    1.01     76.2±1.04ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00     91.7±1.09ms        ? ?/sec    1.00     92.1±0.85ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.00     60.5±1.08ms        ? ?/sec    1.00     60.6±0.98ms        ? ?/sec
bundle/bundle@rome-ts                                               1.00    101.0±1.27ms        ? ?/sec    1.00    100.7±0.86ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.00    182.2±2.79ms        ? ?/sec    1.00    182.8±3.73ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.00    221.4±2.45ms        ? ?/sec    1.02    224.8±4.53ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.00    116.2±1.09ms        ? ?/sec    1.00    116.5±1.03ms        ? ?/sec
bundle/bundle@threejs                                               1.12     36.4±1.11ms        ? ?/sec    1.00     32.7±0.38ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.05     82.9±0.79ms        ? ?/sec    1.00     79.0±0.82ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.04     99.0±1.76ms        ? ?/sec    1.00     94.9±2.24ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.07     45.3±0.38ms        ? ?/sec    1.00     42.2±0.37ms        ? ?/sec
bundle/bundle@threejs10x                                            1.10    387.4±3.03ms        ? ?/sec    1.00    353.4±5.38ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.03    979.8±5.77ms        ? ?/sec    1.00   954.4±10.69ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.01  1217.6±11.49ms        ? ?/sec    1.00  1199.9±13.10ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.08    456.9±5.03ms        ? ?/sec    1.00    421.5±3.14ms        ? ?/sec
remapping/remapping                                                 1.00     32.1±0.17ms        ? ?/sec    1.03     32.9±0.19ms        ? ?/sec
remapping/render-chunk-remapping                                    1.01     82.7±0.42ms        ? ?/sec    1.00     81.5±0.43ms        ? ?/sec
scan/scan@rome-ts                                                   1.01     81.3±0.98ms        ? ?/sec    1.00     80.8±0.87ms        ? ?/sec
scan/scan@threejs                                                   1.13     28.7±0.40ms        ? ?/sec    1.00     25.3±1.30ms        ? ?/sec
scan/scan@threejs10x                                                1.14    288.1±2.04ms        ? ?/sec    1.00    251.7±1.74ms        ? ?/sec

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Aug 10, 2024

CodSpeed Performance Report

Merging #1933 will degrade performances by 17.69%

Comparing inject-plugin (8d3da5c) with main (2939622)

Summary

❌ 10 (👁 10) regressions
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark main inject-plugin Change
👁 bundle@threejs 178.3 ms 208.5 ms -14.49%
👁 bundle@threejs-minify 318.2 ms 347.7 ms -8.49%
👁 bundle@threejs-minify-sourcemap 438.6 ms 467 ms -6.08%
👁 bundle@threejs-sourcemap 226.5 ms 257.9 ms -12.19%
👁 bundle@threejs10x 1.9 s 2.2 s -13.6%
👁 bundle@threejs10x-minify 3.6 s 3.9 s -7.4%
👁 bundle@threejs10x-minify-sourcemap 4.9 s 5.2 s -5.72%
👁 bundle@threejs10x-sourcemap 2.4 s 2.7 s -11.12%
👁 scan@threejs 138.4 ms 168.2 ms -17.69%
👁 scan@threejs10x 1.4 s 1.7 s -17.58%

@hyf0
Copy link
Copy Markdown
Member

hyf0 commented Aug 10, 2024

Any idea about why performance get down so much even we don't use inject?

---edited

Besides adding skipping logic, even we do a full-ast traverse with empty options, it shouldn't slow that much. This PR adds a almost full-ast traverse pass, but have little affection.

@hyf0 hyf0 marked this pull request as ready for review August 10, 2024 17:19
@hyf0 hyf0 changed the title feat(rolldown): add inject plugin feat(rust/rolldown): support inject imports Aug 10, 2024
assert.strictEqual(P, 'promise-shim')
assert.strictEqual($, 'jquery')
assert.strictEqual(fs.default, 'node-fs')
// FIXME: oxc injects invalid statements `import { default as 'Object.assign' from 'object-assign-shim'` }`, so it fails.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Boshen cc

There is a bug here.

I take a rough look

oxc transform to the following code

import { default as 'Object.assign'} from 'object-assign-shim'`

This is invalid js code.

while it should be

import { default as Object_assign} from 'object-assign-shim'`

or

import Object_assign from 'object-assign-shim'`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@hyf0 hyf0 enabled auto-merge August 10, 2024 17:23
@hyf0 hyf0 added this pull request to the merge queue Aug 10, 2024
Merged via the queue into main with commit a2bb2ca Aug 10, 2024
@hyf0 hyf0 deleted the inject-plugin branch August 10, 2024 17:35
@Boshen
Copy link
Copy Markdown
Member Author

Boshen commented Aug 11, 2024

Any idea about why performance get down so much even we don't use inject?

---edited

Besides adding skipping logic, even we do a full-ast traverse with empty options, it shouldn't slow that much. This PR adds a almost full-ast traverse pass, but have little affection.

This is because semantic is now run for each file in this file, where previously its only run for ts or jsx files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants