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

refactor: shared module table at PluginContext #1144

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

underfin
Copy link
Contributor

Description

The pr intend to support PluginContext#getModuleInfo, before i already support NormalModule cast to ModuleInfo, it only work at moduleParesd hook. But the PluginContext#getModuleInfo need to valid at plugin lifetime. I consider two ways to support it.

A. Add a moduleInfos(Dashmap) to owner ModuleInfoat PluginContext, but it need to set the module info when the ModuleInfo fields change, it need to re-set many times, due to https://rollupjs.org/plugin-development/#this-getmoduleinfo. It also has some overhead if the plugin not used it.
B. Shared ModuleTable at PluginContext, the typing is Arc<RwLock<ModuleTable>>, we only need to muate it at module loader and sort modules, it always do it at single thread, so we are using RwLock to avoid caused performance regression if lock conflict. Other, TheModuleInfo will be lazy, if you used it, it will cast from NormalModule.

The pr intend to implement B, it is sound good to me. If you have some good ideas, please let know.

The mutate SharedModuleTable is not finished at module loader, it look like is complex, i intend to do it at next pr.

Copy link

netlify bot commented May 16, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit f80d1d8
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/664608257d49b00008bcffb4

Copy link

netlify bot commented May 16, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 55d727d
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/6646082e0e89090009219e57

Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 93.14286% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 86.90%. Comparing base (130013b) to head (55d727d).
Report is 1 commits behind head on main.

Files Patch % Lines
crates/rolldown_plugin/src/plugin_context.rs 0.00% 8 Missing ⚠️
...stages/generate_stage/compute_cross_chunk_links.rs 90.90% 1 Missing ⚠️
crates/rolldown/src/stages/link_stage/mod.rs 97.87% 1 Missing ⚠️
...s/rolldown/src/utils/chunk/render_chunk_exports.rs 50.00% 1 Missing ⚠️
crates/rolldown_plugin/src/plugin_driver/mod.rs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
- Coverage   86.95%   86.90%   -0.05%     
==========================================
  Files         122      122              
  Lines        6018     6064      +46     
==========================================
+ Hits         5233     5270      +37     
- Misses        785      794       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Benchmarks Rust

group                                      pr                                     target
-----                                      --                                     ------
rolldown benchmark/threejs-bundle          1.00     28.9±0.42ms        ? ?/sec    1.01     29.1±0.38ms        ? ?/sec
rolldown benchmark/threejs-scan            1.03     20.8±1.67ms        ? ?/sec    1.00     20.2±0.09ms        ? ?/sec
rolldown benchmark/threejs-sourcemap       1.02     41.3±1.64ms        ? ?/sec    1.00     40.5±1.01ms        ? ?/sec
rolldown benchmark/threejs10x-bundle       1.01    310.3±2.31ms        ? ?/sec    1.00    308.1±1.87ms        ? ?/sec
rolldown benchmark/threejs10x-scan         1.00    202.2±2.03ms        ? ?/sec    1.01    204.7±4.43ms        ? ?/sec
rolldown benchmark/threejs10x-sourcemap    1.02   448.9±12.95ms        ? ?/sec    1.00   440.0±11.25ms        ? ?/sec

Copy link

codspeed-hq bot commented May 16, 2024

CodSpeed Performance Report

Merging #1144 will not alter performance

Comparing shared-module-table (55d727d) with main (130013b)

Summary

✅ 6 untouched benchmarks

@hyf0 hyf0 added on-hold and removed needs-triage labels May 16, 2024
@hyf0
Copy link
Member

hyf0 commented May 16, 2024

I give a rough look, it's not worth to wrap ModuleTable in Arc<Mutex<...>> just for support this.getModuleInfo.

Compatibility not comes first. If we can't implement a compatible feature in reasonable way, we either try to find other ways or just give up the feature temperately. The compatible feature should not deeply affects the internal core code of rolldown

@underfin
Copy link
Contributor Author

Compatibility not comes first. If we can't implement a compatible feature in reasonable way, we either try to find other ways or just give up the feature temperately. The compatible feature should not deeply affects the internal core code of rolldown

I'm also agree it, I'm not sure for the compatibility, let us discuss it at the meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants