Skip to content

feat(on_demand_wrapping): don't wrap modules that don't rely on others and have side effect#4670

Merged
hyf0 merged 2 commits intomainfrom
05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect
May 25, 2025
Merged

feat(on_demand_wrapping): don't wrap modules that don't rely on others and have side effect#4670
hyf0 merged 2 commits intomainfrom
05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect

Conversation

@hyf0
Copy link
Member

@hyf0 hyf0 commented May 25, 2025 •
edited
Loading

Description

Part of #3410, #3981.

It turns out if a module doesn't have side effect and doesn't rely on other modules(in other words, not resiponsible for initlizing other modules), it could be safely not wrapped.

@hyf0 hyf0 marked this pull request as ready for review May 25, 2025 10:38
@hyf0 hyf0 mentioned this pull request May 25, 2025
Merged
@hyf0 Graphite App
Copy link
Member Author

hyf0 commented May 25, 2025 •
edited
Loading

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

@hyf0 hyf0 force-pushed the 05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect branch from 1869d04 to 942ff7b Compare May 25, 2025 10:43
@hyf0 hyf0 force-pushed the 05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect branch from 942ff7b to 477753c Compare May 25, 2025 10:55
@github-actions
Copy link
Contributor

github-actions bot commented May 25, 2025

Benchmarks Rust

  • target: 05-25-refactor_rust_ensure_module_can_only_be_wrapped_via_wrap_module_recursively_(3017222)
  • pr: 05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect(477753c)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.12     85.6±2.12ms        ? ?/sec    1.00     76.3±1.39ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.10    103.5±2.54ms        ? ?/sec    1.00     94.1±1.07ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00    108.2±0.98ms        ? ?/sec    1.00    108.7±3.60ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.10     95.3±1.60ms        ? ?/sec    1.00     87.0±1.46ms        ? ?/sec
bundle/bundle@rome-ts                                               1.00    115.1±1.38ms        ? ?/sec    1.00    115.4±1.16ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.00    184.5±3.26ms        ? ?/sec    1.01    185.8±2.44ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.15    254.1±7.50ms        ? ?/sec    1.00    221.2±6.61ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.00    125.6±1.20ms        ? ?/sec    1.01    127.3±1.07ms        ? ?/sec
bundle/bundle@threejs                                               1.01     39.6±1.92ms        ? ?/sec    1.00     39.1±1.60ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.02     82.1±1.31ms        ? ?/sec    1.00     80.4±0.40ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.03     99.0±3.29ms        ? ?/sec    1.00     95.9±0.40ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.01     45.7±0.44ms        ? ?/sec    1.00     45.4±0.31ms        ? ?/sec
bundle/bundle@threejs10x                                            1.01    418.4±2.88ms        ? ?/sec    1.00    414.6±2.73ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.01   1004.5±5.37ms        ? ?/sec    1.00    998.8±4.59ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.01   1186.6±9.67ms        ? ?/sec    1.00   1175.4±4.74ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00    479.5±2.91ms        ? ?/sec    1.00    479.8±2.44ms        ? ?/sec
remapping/remapping                                                 1.08     26.5±0.96ms        ? ?/sec    1.00     24.7±2.57ms        ? ?/sec
remapping/render-chunk-remapping                                    1.06     70.7±5.29ms        ? ?/sec    1.00     66.5±0.94ms        ? ?/sec
scan/scan@rome-ts                                                   1.00     89.7±1.13ms        ? ?/sec    1.01     90.7±0.99ms        ? ?/sec
scan/scan@threejs                                                   1.00     30.5±1.09ms        ? ?/sec    1.02     31.0±0.96ms        ? ?/sec
scan/scan@threejs10x                                                1.00    306.3±4.72ms        ? ?/sec    1.00    304.9±2.98ms        ? ?/sec

Base automatically changed from 05-25-refactor_rust_ensure_module_can_only_be_wrapped_via_wrap_module_recursively_ to main May 25, 2025 15:55
@hyf0 hyf0 added this pull request to the merge queue May 25, 2025
Merged via the queue into main with commit 326e6f5 May 25, 2025
28 checks passed
@hyf0 hyf0 deleted the 05-25-feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect branch May 25, 2025 16:07
@sapphi-red sapphi-red mentioned this pull request May 26, 2025
Closed
hyf0 added a commit that referenced this pull request May 26, 2025
@hyf0
revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…
2654112 
… on others and have side effect #4670`
hyf0 added a commit that referenced this pull request May 26, 2025
@hyf0
revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…
9f1f3e9 
… on others and have side effect #4670`
hyf0 added a commit that referenced this pull request May 26, 2025
@hyf0
revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…
1626cb5 
… on others and have side effect #4670`
hyf0 added a commit that referenced this pull request May 26, 2025
@hyf0
revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…
221794c 
… on others and have side effect #4670`
github-merge-queue bot pushed a commit that referenced this pull request May 26, 2025
@hyf0
revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…
8a77ce9 
… on others and have side effect #4670` (#4686)

<!-- Thank you for contributing! -->

### Description

Close #4684.

---

No side effect is not the full condition to safely remove the wrapper. A
module might not have side effects, but it might rely on the order that
side-effect module get executed first. See the case at
#4684.

<!-- Please insert your description here and provide especially info
about the "what" this PR is solving -->
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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hyf0 @IWANABETHATGUY