Skip to content

[WIP] [v2] Implement string interning and apply to IR terminals and file IDs internally#1564

Draft
ggiraldez wants to merge 5 commits intoggiraldez/v2-migrate-astfrom
ggiraldez/v2-identifier-interning
Draft

[WIP] [v2] Implement string interning and apply to IR terminals and file IDs internally#1564
ggiraldez wants to merge 5 commits intoggiraldez/v2-migrate-astfrom
ggiraldez/v2-identifier-interning

Conversation

@ggiraldez
Copy link
Copy Markdown
Contributor

@ggiraldez ggiraldez commented Mar 20, 2026 •
edited
Loading

Applies on top of #1597

This PR implements interning for the strings of terminals and file IDs used inside the semantic crate. This should improve performance of the binder for the small price of adding a field to all identifiers in the IR.

@ggiraldez ggiraldez requested review from OmarTawfik and teofr March 20, 2026 00:03
@ggiraldez ggiraldez requested review from a team as code owners March 20, 2026 00:03
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 20, 2026 •
edited
Loading

⚠️ No Changeset found

Latest commit: 03ea285

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

teofr
teofr reviewed Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@teofr teofr left a comment

Choose a reason for hiding this comment

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

Quick look at it

@ggiraldez ggiraldez requested a review from teofr March 20, 2026 22:15
@ggiraldez ggiraldez force-pushed the ggiraldez/v2-identifier-interning branch from 4afadf0 to fbfdf6f Compare March 20, 2026 22:57
@ggiraldez ggiraldez force-pushed the ggiraldez/v2-normalize-yul-terminals branch from b80d93f to 361d6b5 Compare March 23, 2026 16:57
@ggiraldez ggiraldez force-pushed the ggiraldez/v2-identifier-interning branch from fbfdf6f to 46800a4 Compare March 23, 2026 19:54
OmarTawfik
OmarTawfik reviewed Mar 24, 2026
View reviewed changes
crates/solidity-v2/outputs/cargo/semantic/src/interner.rs Outdated
set: IndexSet<String>,
}

impl Interner {
Copy link
Copy Markdown
Contributor

@OmarTawfik OmarTawfik Mar 24, 2026

Choose a reason for hiding this comment

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

PR Looking great!
But for both the overall interner + the other comment about checking before cloning, I suggest doing a perf comparison with iai, to get some concrete numbers with real-world inputs.

Copy link
Copy Markdown
Contributor

@teofr teofr Mar 27, 2026

Choose a reason for hiding this comment

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

I'm not sure if we're still going forward with this PR, but I think #1572 is enough to benchmark it

@ggiraldez ggiraldez force-pushed the ggiraldez/v2-normalize-yul-terminals branch from 6362467 to d7f0eff Compare March 24, 2026 16:11
Base automatically changed from ggiraldez/v2-normalize-yul-terminals to main March 24, 2026 16:33
@ggiraldez ggiraldez force-pushed the ggiraldez/v2-identifier-interning branch from 46800a4 to 2820353 Compare March 24, 2026 16:46
@ggiraldez ggiraldez marked this pull request as draft March 27, 2026 17:01
@ggiraldez
Copy link
Copy Markdown
Contributor Author

ggiraldez commented Mar 27, 2026

Moved this PR back to draft since this is a performance improvement and not priority right now.

@ggiraldez ggiraldez force-pushed the ggiraldez/v2-identifier-interning branch from 2820353 to 9f0c15d Compare April 10, 2026 20:41
@ggiraldez ggiraldez changed the base branch from main to ggiraldez/v2-migrate-ast April 10, 2026 20:42
@ggiraldez ggiraldez changed the title [v2] Implement Identifier string interning [v2] Implement string interning and apply to Identifiers and file IDs internally Apr 10, 2026
@ggiraldez ggiraldez changed the title [v2] Implement string interning and apply to Identifiers and file IDs internally [v2] Implement string interning and apply to IR terminals and file IDs internally Apr 10, 2026
@ggiraldez
Copy link
Copy Markdown
Contributor Author

ggiraldez commented Apr 10, 2026

I rebased and continued this implementation on top of the last semantic V2 migration PR (#1597).

We can add the ci-perf label after #1666 is merged to have a baseline to compare against.

@ggiraldez ggiraldez force-pushed the ggiraldez/v2-migrate-ast branch from e917ee5 to 592548c Compare April 13, 2026 15:34
@ggiraldez ggiraldez force-pushed the ggiraldez/v2-identifier-interning branch from 9f0c15d to 03ea285 Compare April 13, 2026 15:51
@ggiraldez ggiraldez changed the title [v2] Implement string interning and apply to IR terminals and file IDs internally [WIP] [v2] Implement string interning and apply to IR terminals and file IDs internally Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmarTawfik OmarTawfik OmarTawfik left review comments

@teofr teofr Awaiting requested review from teofr

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ggiraldez @teofr @OmarTawfik