Skip to content

perf: Provide better incrementality for modules#22322

Open
ChayimFriedman2 wants to merge 1 commit into
rust-lang:masterfrom
ChayimFriedman2:module-inc
Open

perf: Provide better incrementality for modules#22322
ChayimFriedman2 wants to merge 1 commit into
rust-lang:masterfrom
ChayimFriedman2:module-inc

Conversation

@ChayimFriedman2
Copy link
Copy Markdown
Contributor

Previously module ID reuse was based on order of creation solely. Now we store the parent module and name, and so Salsa will reuse the ID for modules that have the same name and parent.

This is important as many interned IDs contain modules, so if the module is invalidated they too are.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2026
Copy link
Copy Markdown
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Comment thread crates/hir-def/src/lib.rs
pub block: Option<BlockId>,
/// The parent module of this module, or `None` if this is the root module inside the def
/// map (including for block def maps).
pub containing_module_inside_def_map: Option<ModuleIdLt<'db>>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why not just call this parent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to be explicit. But I can change that, if you want.

Comment thread crates/hir-def/src/lib.rs

pub fn name(self, db: &dyn DefDatabase) -> Option<Name> {
let def_map = self.def_map(db);
let parent = def_map[self].parent?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need the parent field on the ModuleData now or can we discard this in favor of the IDs field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's still used in one place but we can derive it from the module instead of the def map.

Comment thread crates/hir-def/src/lib.rs
/// map (including for block def maps).
pub containing_module_inside_def_map: Option<ModuleIdLt<'db>>,
/// The name of this module, or [`sym::__empty`] for the root module.
name_or_empty: Name,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would also call this just name, keeping the doc comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This will clash with the method (because Salsa generates a method), a frankly I'm happier with the current name. It removes any chances of mistake.

@rustbot

This comment has been minimized.

Previously module ID reuse was based on order of creation solely. Now we store the parent module and name, and so Salsa will reuse the ID for modules that have the same name and parent.

This is important as many interned IDs contain modules, so if the module is invalidated they too are.
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 11, 2026

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants