Skip to content

Comments

perf(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations#10058

Merged
graphite-app[bot] merged 1 commit intomainfrom
03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations
Mar 28, 2025
Merged

perf(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations#10058
graphite-app[bot] merged 1 commit intomainfrom
03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Mar 26, 2025 •
edited
Loading

Further improve on #4463.

As #4463 said:

Most symbols don't have redeclarations.

Yes, so inserting a None for every SymbolId is also unnecessary, changes to FxHashMap so that we can insert where redeclaration appears.

There is a 1% performance improvement in semantic!

image

@github-actions github-actions bot added A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Mar 26, 2025
@Dunqing Graphite App
Copy link
Member Author

Dunqing commented Mar 26, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 26, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #10058 will not alter performance

Comparing 03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations (84e167e) with main (5e14fe9)

Summary

✅ 33 untouched benchmarks

@Dunqing Dunqing changed the title refactor(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations perf(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations Mar 26, 2025
@Dunqing Dunqing changed the title perf(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations pref(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations Mar 26, 2025
@Dunqing Dunqing changed the title pref(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations perf(semantic): use FxHashMap to replace IndexVec for symbol_redeclarations Mar 26, 2025
@github-actions github-actions bot added the C-performance Category - Solution not expected to change functional behavior, only performance label Mar 26, 2025
@Dunqing Dunqing force-pushed the 03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations branch from 814a4be to e507c56 Compare March 26, 2025 07:13
This was referenced Mar 26, 2025
Merged
Merged
@Dunqing Dunqing force-pushed the 03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations branch from e507c56 to 0a6af60 Compare March 26, 2025 08:47
This was referenced Mar 26, 2025
Merged
Merged
Merged
@Dunqing Dunqing marked this pull request as ready for review March 27, 2025 12:34
@Dunqing Dunqing requested a review from Boshen as a code owner March 27, 2025 12:34
This was referenced Mar 27, 2025
Merged
Merged
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Mar 28, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Mar 28, 2025 •
edited
Loading

Merge activity

@Dunqing
perf(semantic): use FxHashMap to replace IndexVec for `symbol_red…
84e167e 
…eclarations` (#10058)

Further improve on #4463.

As #4463 said:

> Most symbols don't have redeclarations.

Yes, so inserting a `None` for every `SymbolId` is also unnecessary, changes to `FxHashMap` so that we can insert where redeclaration appears.

There is a 1% performance improvement in semantic!

<img width="996" alt="image" src="https://github.com/user-attachments/assets/612aea11-8555-4734-a453-443194413d21" />
@graphite-app graphite-app bot force-pushed the 03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations branch from 0a6af60 to 84e167e Compare March 28, 2025 04:12
@graphite-app graphite-app bot merged commit 84e167e into main Mar 28, 2025
27 checks passed
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 28, 2025
@graphite-app graphite-app bot deleted the 03-25-refactor_semantic_use_fxhashmap_to_replace_indexvec_for_symbol_redeclarations branch March 28, 2025 04:18
This was referenced Mar 28, 2025
Merged
Merged
Merged
@oxc-bot oxc-bot mentioned this pull request Apr 1, 2025
Merged
@overlookmotel
Copy link
Member

overlookmotel commented Apr 3, 2025 •
edited
Loading

I'm late to the party on this one but: Nice!

I'm not sure why I used the IndexVec<SymbolId, Option<RedeclarationId>> in the first place. A hash map is much better, for the reasons you gave.

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

Reviewers

@Boshen Boshen Awaiting requested review from Boshen

Assignees

No one assigned

Labels

A-semantic Area - Semantic C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dunqing @overlookmotel