Skip to content

feat(rust): support define functionality#1882

Merged
hyf0 merged 1 commit intomainfrom
08-07-feat_rust_support_define_functionality
Aug 6, 2024
Merged

feat(rust): support define functionality#1882
hyf0 merged 1 commit intomainfrom
08-07-feat_rust_support_define_functionality

Conversation

@hyf0
Copy link
Copy Markdown
Member

@hyf0 hyf0 commented Aug 6, 2024 •
edited
Loading

@hyf0 hyf0 mentioned this pull request Aug 6, 2024
Merged
@hyf0 Graphite App
Copy link
Copy Markdown
Member Author

hyf0 commented Aug 6, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hyf0 and the rest of your teammates on Graphite Graphite

@hyf0 hyf0 mentioned this pull request Aug 6, 2024
Merged
@hyf0 hyf0 marked this pull request as ready for review August 6, 2024 16:30
@hyf0 hyf0 self-assigned this Aug 6, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 6, 2024 •
edited
Loading

Benchmarks Rust

  • target: main(9b3147a)
  • pr: 08-07-feat_rust_support_define_functionality(24a357e)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.04     49.3±1.44ms        ? ?/sec    1.00     47.5±0.50ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.02     66.4±0.85ms        ? ?/sec    1.00     65.2±0.84ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.01     83.4±1.10ms        ? ?/sec    1.00     82.9±0.97ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.04     58.5±1.53ms        ? ?/sec    1.00     56.3±0.62ms        ? ?/sec
bundle/bundle@rome-ts                                               1.05     96.2±2.18ms        ? ?/sec    1.00     91.4±0.72ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.08    166.9±1.71ms        ? ?/sec    1.00    154.0±4.31ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.02    202.6±3.44ms        ? ?/sec    1.00    198.9±3.98ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.05    112.8±1.31ms        ? ?/sec    1.00    107.2±0.93ms        ? ?/sec
bundle/bundle@threejs                                               1.04     28.5±0.61ms        ? ?/sec    1.00     27.3±0.32ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.03     66.6±3.45ms        ? ?/sec    1.00     64.7±1.06ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.00     81.7±1.99ms        ? ?/sec    1.00     81.6±1.58ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.02     37.2±1.58ms        ? ?/sec    1.00     36.5±1.07ms        ? ?/sec
bundle/bundle@threejs10x                                            1.03    314.5±4.19ms        ? ?/sec    1.00    306.3±3.67ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.02    777.0±9.88ms        ? ?/sec    1.00   762.3±12.53ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.02  1018.2±12.96ms        ? ?/sec    1.00   997.9±16.07ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.04    389.0±5.82ms        ? ?/sec    1.00    375.0±3.79ms        ? ?/sec
remapping/remapping                                                 1.02     33.0±0.12ms        ? ?/sec    1.00     32.4±0.13ms        ? ?/sec
remapping/render-chunk-remapping                                    1.01     82.5±0.44ms        ? ?/sec    1.00     81.9±0.48ms        ? ?/sec
scan/scan@rome-ts                                                   1.04     74.8±0.99ms        ? ?/sec    1.00     71.6±0.62ms        ? ?/sec
scan/scan@threejs                                                   1.06     21.2±0.50ms        ? ?/sec    1.00     19.9±0.38ms        ? ?/sec
scan/scan@threejs10x                                                1.03    210.6±1.43ms        ? ?/sec    1.00    205.3±2.40ms        ? ?/sec

@hyf0 hyf0 force-pushed the 08-07-test_rust_allow_to_write_comments_in__config.json_ branch from 31895b8 to 9a914a9 Compare August 6, 2024 16:51
@hyf0 hyf0 force-pushed the 08-07-feat_rust_support_define_functionality branch from ac24f6f to b1a03f1 Compare August 6, 2024 16:52
IWANABETHATGUY
IWANABETHATGUY reviewed Aug 6, 2024
View reviewed changes
Comment thread crates/rolldown/src/utils/pre_process_ecma_ast.rs
errs
)
})?;
ReplaceGlobalDefines::new(fields.allocator, define_config).build(fields.program);
Copy link
Copy Markdown
Member

@IWANABETHATGUY IWANABETHATGUY Aug 6, 2024

Choose a reason for hiding this comment

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

Construct the ReplaceGlobalConfig looks none trivial https://github.com/oxc-project/oxc/blob/910f2a0a0a7a4298923afc88511d089843d02d3a/crates/oxc_minifier/src/ast_passes/replace_global_defines.rs#L30-L54, do we need to share them with Arc?

Copy link
Copy Markdown
Member Author

@hyf0 hyf0 Aug 6, 2024

Choose a reason for hiding this comment

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

Good catch. @Boshen cc

Copy link
Copy Markdown
Member Author

@hyf0 hyf0 Aug 6, 2024

Choose a reason for hiding this comment

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

Guess this also requires oxc-project/oxc#4284.

Copy link
Copy Markdown
Member

@Boshen Boshen Aug 8, 2024

Choose a reason for hiding this comment

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

https://github.com/oxc-project/oxc/blob/69dc6c9cb1f93209e5a0bc003918d1bc6f50e455/crates/oxc_minifier/src/plugins/replace_global_defines.rs#L17 it's Arced.

Copy link
Copy Markdown
Member

@IWANABETHATGUY IWANABETHATGUY Aug 8, 2024

Choose a reason for hiding this comment

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

Yeah, the Defiens is Arced in oxc side,but we still create the DefineConfig per file.

@hyf0 hyf0 mentioned this pull request Aug 6, 2024
Merged
Base automatically changed from 08-07-test_rust_allow_to_write_comments_in__config.json_ to main August 6, 2024 17:19
@hyf0 hyf0 force-pushed the 08-07-feat_rust_support_define_functionality branch from b1a03f1 to 24a357e Compare August 6, 2024 17:22
@hyf0 hyf0 mentioned this pull request Aug 6, 2024
Merged
@netlify
Copy link
Copy Markdown

netlify Bot commented Aug 6, 2024

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 24a357e
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66b25be812b7d8000816c046

@hyf0 hyf0 added this pull request to the merge queue Aug 6, 2024
Merged via the queue into main with commit 8639e70 Aug 6, 2024
@hyf0 hyf0 deleted the 08-07-feat_rust_support_define_functionality branch August 6, 2024 17:45
@shulaoda shulaoda mentioned this pull request Aug 7, 2024
Closed
@hyf0 hyf0 mentioned this pull request Aug 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Boshen Boshen Boshen left review comments

@IWANABETHATGUY IWANABETHATGUY IWANABETHATGUY left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees

@hyf0 hyf0

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hyf0 @Boshen @IWANABETHATGUY