Merged
Conversation
Merged
Morriar
approved these changes
Jan 30, 2026
| #[derive(PartialEq, Eq, Debug, Clone, Copy)] | ||
| pub struct DeclarationMarker; | ||
| /// `DeclarationId` represents the ID of a fully qualified name. For example, `Foo::Bar` or `Foo#my_method` | ||
| pub type DeclarationId = Id<DeclarationMarker>; |
Contributor
There was a problem hiding this comment.
Should we also tag declarations?
Member
Author
There was a problem hiding this comment.
I haven't figure out a good way to do it. Consider our Ruby API:
graph["Foo"]The Ruby API has no way of knowing what Foo is before looking it up, so we wouldn't be able to construct the right DeclarationId to search.
For now, I think it's okay because we have a lot fewer declarations than definitions.
| } | ||
| } | ||
|
|
||
| #[repr(u8)] |
Contributor
There was a problem hiding this comment.
You should be able to fit this in 4 bits?
Member
Author
There was a problem hiding this comment.
Is there a way to do this? u8 is the smallest integer storage we can use no?
| offset::Offset, | ||
| }; | ||
|
|
||
| #[repr(u8)] |
9d72fcd to
f486994
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Step towards #141
When thinking about using u32 IDs, I was getting really caught up by the fact that Rust's
HashMapimplementation still requires storing a u64 for the key digests. Of course that consumes more memory and ideally we can have a customHashMapimplementation that uses half the amount of bits.However, we actually use IDs quite extensively outside of just
HashMapkeys. For example, many definitions store StringId, names store multiple NameIds and ancestors is a list of DeclarationId. We can actually have very substantial memory savings right now even without the 32 bitHashMap.Implementation
This PR reduces the size of our IDs to u32 from i64. I recommend reviewing per commit:
Reduce the ID size to u32
Allow
DefinitionIdandReferenceIdto be tagged with their kind. This helps us reduce the changes of collision by encoding the kind in the lower bits of the ID. Essentially, we have 28 bits for the digest + 4 for the kind. Definitions and references are the things we have the most in Core and the only ones I got conflicts for running the release mode, so I think this is enough for the time being.As an added benefit, this allows to check the kind of definitions and references without having to retrieve them from the graph. That information is directly encoded in the ID, we can just invoke
kind. Note that thekindaddition conflicts with Provide resolution diagnostics (inline) #502, we can let that PR go first.Adjust the C side to use u32
Impact
Despite still using 64 bit
HashMaps, this PR still reduces our memory used from~3810 MBto~2880 MB(25% reduction).I believe we can further reduce this with a custom
HashMapimplementation that stores u32 internally.Why now?
I believe this is the right time to do this for the following reason: we're starting adoption of Rubydex in our tools. It will be easier for us to verify that we can indeed get away with u32 and lower memory usage and then increase to u64 if necessary than the other way around.
Also, we're almost getting to 4GB and a reduction is definitely welcome.