Skip to content

fix: infer async modules when incremental enabled#7927

Merged
ahabhgk merged 6 commits intomainfrom
fix-infer-async
Sep 20, 2024
Merged

fix: infer async modules when incremental enabled#7927
ahabhgk merged 6 commits intomainfrom
fix-infer-async

Conversation

@ahabhgk
Copy link
Copy Markdown
Contributor

@ahabhgk ahabhgk commented Sep 19, 2024

Summary

fix infer async modules when incremental enabled (both incremental and newIncremental)

This could cause a little bit perf regression when cold start but correctness first and the regression is acceptable (under 5% according to the bench) shouldn't cause perf regression

Checklist

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

@ahabhgk ahabhgk requested a review from JSerFeng September 19, 2024 09:43
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Sep 19, 2024
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Sep 19, 2024
@netlify
Copy link
Copy Markdown

netlify bot commented Sep 19, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 6d392a9
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66ed52f96d23770008a46e3b

@ahabhgk
Copy link
Copy Markdown
Contributor Author

ahabhgk commented Sep 19, 2024

!bench

@rspack-bot
Copy link
Copy Markdown

rspack-bot commented Sep 19, 2024

📝 Benchmark detail: Open

Name Base (2024-09-19 0a89e43) Current Change
10000_development-mode + exec 2.22 s ± 58 ms 2.23 s ± 40 ms +0.68 %
10000_development-mode_hmr + exec 683 ms ± 3.5 ms 699 ms ± 21 ms +2.34 %
10000_production-mode + exec 2.85 s ± 30 ms 2.83 s ± 31 ms -0.71 %
arco-pro_development-mode + exec 1.81 s ± 72 ms 1.84 s ± 77 ms +1.32 %
arco-pro_development-mode_hmr + exec 433 ms ± 2.3 ms 435 ms ± 2.5 ms +0.56 %
arco-pro_production-mode + exec 3.27 s ± 41 ms 3.28 s ± 50 ms +0.27 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec 3.29 s ± 97 ms 3.32 s ± 78 ms +0.96 %
threejs_development-mode_10x + exec 1.66 s ± 19 ms 1.68 s ± 10 ms +0.95 %
threejs_development-mode_10x_hmr + exec 791 ms ± 16 ms 788 ms ± 9.5 ms -0.43 %
threejs_production-mode_10x + exec 5.14 s ± 22 ms 5.18 s ± 36 ms +0.74 %

@ahabhgk ahabhgk enabled auto-merge (squash) September 19, 2024 10:31
JSerFeng
JSerFeng previously approved these changes Sep 19, 2024
@JSerFeng JSerFeng dismissed their stale review September 19, 2024 10:57

have questions

@JSerFeng
Copy link
Copy Markdown
Contributor

Can you explain the issue before ?

@ahabhgk
Copy link
Copy Markdown
Contributor Author

ahabhgk commented Sep 19, 2024

for example, a -> b -> c

in the first build, c has top-level-await, this will infer a and b are async modules (a (async) -> b (async) -> c (async))

but in the second build, we remove the top-level-await inside c, the correct module graph should be a -> b -> c, but since we incrementally rebuild the module graph, only rebuild the modified module c, so the module graph is a (async) -> b (async) -> c, this is an incorrect module graph

in this PR we will fix this first in InferAsyncModulesPlugin, we find the not has_top_level_await modules from affected modules, and infer is_async for them, reset is_async back to false first, and then find the has_top_level_await modules from affected modules and set them and there inferred modules to is_async: true

and when newIncremental disabled, old incremental enabled or nothing enabled at all, the affected modules is just all modules, follow the same steps, will also have a correctly updated module graph

@ahabhgk ahabhgk requested a review from JSerFeng September 19, 2024 11:30
@JSerFeng
Copy link
Copy Markdown
Contributor

for example, a -> b -> c

in the first build, c has top-level-await, this will infer a and b are async modules (a (async) -> b (async) -> c (async))

Why are a and b not included in affectedModules, as they have data referred from c

@ahabhgk ahabhgk disabled auto-merge September 20, 2024 07:58
@ahabhgk ahabhgk enabled auto-merge (squash) September 20, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants