Skip to content

Comments

refactor(module_loader): use HybridIndexVec::update for value assignment#7755

Closed
shulaoda wants to merge 1 commit intomainfrom
01-05-refactor_use_hybridindexvec_update_for_value_assignment
Closed

refactor(module_loader): use HybridIndexVec::update for value assignment#7755
shulaoda wants to merge 1 commit intomainfrom
01-05-refactor_use_hybridindexvec_update_for_value_assignment

Conversation

@shulaoda
Copy link
Member

@shulaoda shulaoda commented Jan 5, 2026 •
edited
Loading

  • Add HybridIndexVec::update method for updating values at existing indices
  • Replace *get_mut(idx) = value with update(idx, value)
  • Use explicit Module::External(Box::new(...)) instead of .into()

@shulaoda Graphite App
Copy link
Member Author

shulaoda commented Jan 5, 2026 •
edited
Loading

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@shulaoda shulaoda mentioned this pull request Jan 5, 2026
Merged
@shulaoda shulaoda marked this pull request as ready for review January 5, 2026 11:29
@shulaoda shulaoda changed the title refactor: use HybridIndexVec::update for value assignment refactor: use HybridIndexVec::update for value assignment Jan 5, 2026
@shulaoda shulaoda mentioned this pull request Jan 5, 2026
Merged
@shulaoda shulaoda force-pushed the 01-05-refactor_use_hybridindexvec_update_for_value_assignment branch from 3b7ff66 to e82b8de Compare January 5, 2026 11:53
@shulaoda shulaoda changed the title refactor: use HybridIndexVec::update for value assignment refactor(module_loader): use HybridIndexVec::update for value assignment Jan 5, 2026
@graphite-app graphite-app bot changed the base branch from 01-05-refactor_remove_unnecessary_dynamic_import_handling_for_runtime_module to graphite-base/7755 January 5, 2026 12:09
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2026 •
edited
Loading

Benchmarks Rust

  • target: main(fac5b13)
  • pr: 01-05-refactor_use_hybridindexvec_update_for_value_assignment(ad3e134)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.04     64.5±1.43ms        ? ?/sec    1.00     62.1±1.47ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.03     70.7±1.71ms        ? ?/sec    1.00     69.0±2.07ms        ? ?/sec
bundle/bundle@rome_ts                                        1.02    107.1±2.34ms        ? ?/sec    1.00    105.4±2.37ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.03    120.0±4.68ms        ? ?/sec    1.00    116.7±1.74ms        ? ?/sec
bundle/bundle@threejs                                        1.01     38.5±2.26ms        ? ?/sec    1.00     38.1±0.79ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     42.9±0.91ms        ? ?/sec    1.00     42.3±0.58ms        ? ?/sec
bundle/bundle@threejs10x                                     1.01    388.9±6.85ms        ? ?/sec    1.00    385.1±4.48ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    447.5±5.22ms        ? ?/sec    1.00    446.4±3.92ms        ? ?/sec
scan/scan@rome_ts                                            1.00     83.0±1.51ms        ? ?/sec    1.04     86.3±1.77ms        ? ?/sec
scan/scan@threejs                                            1.00     28.5±1.75ms        ? ?/sec    1.00     28.5±0.54ms        ? ?/sec
scan/scan@threejs10x                                         1.00    291.2±4.62ms        ? ?/sec    1.01    294.6±4.29ms        ? ?/sec

IWANABETHATGUY
IWANABETHATGUY reviewed Jan 5, 2026
View reviewed changes
crates/rolldown_common/src/types/hybrid_index_vec.rs
*map.get_mut(&i).expect("should have idx") = value;
}
}
}
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY Jan 5, 2026

Choose a reason for hiding this comment

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

Could you explain why we need this indirect method?

Copy link
Member Author

@shulaoda shulaoda Jan 6, 2026

Choose a reason for hiding this comment

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

It doesn’t seem particularly necessary, it’s largely a matter of style. I probably overthought it, so I’ll close this.

@graphite-app graphite-app bot force-pushed the graphite-base/7755 branch from 3116949 to fac5b13 Compare January 5, 2026 12:22
@graphite-app graphite-app bot force-pushed the 01-05-refactor_use_hybridindexvec_update_for_value_assignment branch from e82b8de to 4bf3df2 Compare January 5, 2026 12:22
@graphite-app graphite-app bot changed the base branch from graphite-base/7755 to main January 5, 2026 12:22
@graphite-app graphite-app bot force-pushed the 01-05-refactor_use_hybridindexvec_update_for_value_assignment branch from 4bf3df2 to ad3e134 Compare January 5, 2026 12:23
@netlify
Copy link

netlify bot commented Jan 5, 2026

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit ad3e134
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/695bad26aeef3700088e34e1

@shulaoda shulaoda closed this Jan 6, 2026
@shulaoda shulaoda deleted the 01-05-refactor_use_hybridindexvec_update_for_value_assignment branch January 6, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@IWANABETHATGUY IWANABETHATGUY IWANABETHATGUY left review comments

@hyf0 hyf0 Awaiting requested review from hyf0

Assignees

@shulaoda shulaoda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@shulaoda @IWANABETHATGUY