Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: root module is less prone to be wrapped in IIFE #6697

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

fi3ework
Copy link
Member

@fi3ework fi3ework commented May 31, 2024

Summary

Sync up porting webpack/webpack#18348 and webpack/webpack#18349.

Main changes:

  • Extract find_new_name from method to function
  • Remove conflicts between through variables in non-inlined modules and module scope inlined modules which will be accessible from non-inlined modules after module renaming. The renaming logic is heavily based on the existing concatenated methods.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label May 31, 2024
Copy link

netlify bot commented May 31, 2024

Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 9131758
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66728b76606c180008da693e

@fi3ework
Copy link
Member Author

fi3ework commented May 31, 2024

@ahabhgk
Copy link
Collaborator

ahabhgk commented Jun 11, 2024

!bench

@rspack-bot
Copy link

rspack-bot commented Jun 11, 2024

📝 Benchmark detail: Open

Name Base (2024-06-11 34bb9ea) Current Change
10000_development-mode + exec 2.24 s ± 26 ms 2.22 s ± 16 ms -0.98 %
10000_development-mode_hmr + exec 737 ms ± 14 ms 732 ms ± 12 ms -0.66 %
10000_production-mode + exec 2.58 s ± 21 ms 2.56 s ± 24 ms -0.69 %
arco-pro_development-mode + exec 1.92 s ± 78 ms 1.95 s ± 59 ms +1.58 %
arco-pro_development-mode_hmr + exec 441 ms ± 1.3 ms 441 ms ± 1.9 ms +0.09 %
arco-pro_production-mode + exec 3.51 s ± 71 ms 3.52 s ± 93 ms +0.30 %
threejs_development-mode_10x + exec 1.41 s ± 15 ms 2.02 s ± 14 ms +43.27 %
threejs_development-mode_10x_hmr + exec 802 ms ± 6.9 ms 1.41 s ± 11 ms +76.05 %
threejs_production-mode_10x + exec 4.71 s ± 20 ms 5.3 s ± 34 ms +12.56 %

Threshold exceeded: ["threejs_development-mode_10x + exec","threejs_development-mode_10x_hmr + exec","threejs_production-mode_10x + exec"]

@ahabhgk
Copy link
Collaborator

ahabhgk commented Jun 11, 2024

Generally looks good to me, the benchmark shows in an actual case (arco-pro) this is acceptable, but performance has regressed significantly when there are lots of javascript module (threejs 10x), can this be further improved?

@fi3ework
Copy link
Member Author

@ahabhgk
The overheads could be reduced from two parts:

  1. In development and production, it needs to parse all modules at the first build. I think the most time cost on AST parse from the scratch, I took a look in getting AST from module but didn't find. Is there a way to do that?
  2. In development, we could use a parsed result cache map and reuse only re-parse the module only when modified.
    Any other suggestion?

@ahabhgk
Copy link
Collaborator

ahabhgk commented Jun 17, 2024

There isn't a way to get the AST from the module, we don't store the AST on the module, we analyze dependencies from AST (for javascript) then drop it, especially you need to parse the generated code of the module (source has been modified from the original source, so does AST)

@ahabhgk
Copy link
Collaborator

ahabhgk commented Jun 17, 2024

For caching, something like https://github.com/webpack/webpack/blob/34e2561addb0f65a7a6fb0ce7ae1aea4cd1d599f/lib/javascript/JavascriptModulesPlugin.js#L592 but works for caching the inline module result looks good to me

@fi3ework
Copy link
Member Author

!bench

@rspack-bot
Copy link

rspack-bot commented Jun 18, 2024

📝 Benchmark detail: Open

Name Base (2024-06-18 553f785) Current Change
10000_development-mode + exec 2.21 s ± 22 ms 2.25 s ± 23 ms +1.43 %
10000_development-mode_hmr + exec 737 ms ± 12 ms 743 ms ± 8.4 ms +0.81 %
10000_production-mode + exec 2.57 s ± 22 ms 2.62 s ± 31 ms +1.85 %
arco-pro_development-mode + exec 1.94 s ± 68 ms 1.93 s ± 66 ms -0.29 %
arco-pro_development-mode_hmr + exec 441 ms ± 2.1 ms 442 ms ± 1.6 ms +0.13 %
arco-pro_production-mode + exec 3.53 s ± 85 ms 3.57 s ± 58 ms +1.34 %
threejs_development-mode_10x + exec 1.46 s ± 11 ms 2.09 s ± 15 ms +42.95 %
threejs_development-mode_10x_hmr + exec 799 ms ± 3.3 ms 840 ms ± 6.8 ms +5.22 %
threejs_production-mode_10x + exec 4.76 s ± 33 ms 5.36 s ± 12 ms +12.58 %

Threshold exceeded: ["threejs_development-mode_10x + exec","threejs_development-mode_10x_hmr + exec","threejs_production-mode_10x + exec"]

@fi3ework
Copy link
Member Author

@ahabhgk
The benchmark got better, especially in HRM case. I tested it locally, the upper is with cache and below without. Seen that following HMR speed got faster as cache hits.

image

ahabhgk
ahabhgk previously approved these changes Jun 18, 2024
@ahabhgk
Copy link
Collaborator

ahabhgk commented Jun 18, 2024

Something wrong in our CI, @SyMind is looking at it

@fi3ework
Copy link
Member Author

Patched a commit lost in force-pushing that fix the wrong usage of ReplaceSource, test cases're updated alongside.

@fi3ework
Copy link
Member Author

@SyMind Still failing with a retry just now, is this CI issue still being resolved? 🥲

@SyMind
Copy link
Member

SyMind commented Jun 19, 2024

@fi3ework I am trying to update the branch to get the latest code. Please wait for the CI to run again.

@fi3ework
Copy link
Member Author

@ahabhgk The review was dismissed, PTAL again.

@ahabhgk ahabhgk merged commit 7f9b563 into web-infra-dev:main Jun 19, 2024
28 checks passed
@fi3ework fi3ework deleted the iife branch June 19, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: feature release: feature related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants