Skip to content

Proper name interning #15590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Veykril opened this issue Sep 9, 2023 · 0 comments · Fixed by #17584
Closed

Proper name interning #15590

Veykril opened this issue Sep 9, 2023 · 0 comments · Fixed by #17584
Assignees
Labels
A-perf performance issues C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) E-hard

Comments

@Veykril
Copy link
Member

Veykril commented Sep 9, 2023

Currently our Names are just SmolStrings, which are 24 bytes in size which is pretty massive. They are also not interned at all, merely a fancy Arc so we can (and most likely do) have a lot of the same names allocated in different Arcs as well.

It's unclear (to me at least) what a good solution here is (that would make things a bit more performant). We can't do it the "standard way" of just interning things (into a global/singleton) and returning an index, as that would effectively leak memory over time, so the most likely thing to do would be to probably stick with an Arc, do by identity equality and properly intern through an interner to deduplicate. If we go with a thin arc we'd cut down the size of Names to 8 bytes.

Another option would be to use salsa for interning. This might work fine but I fear that will have a pretty severe performance hit, as we are already struggling with salsa taking a long time to revalidate queries.

@Veykril Veykril added E-hard C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) A-perf performance issues labels Sep 9, 2023
@Veykril Veykril self-assigned this Sep 22, 2023
@bors bors closed this as completed in 5784915 Jul 15, 2024
lnicola pushed a commit to lnicola/rust that referenced this issue Jul 28, 2024
Implement symbol interning infra

Will fix rust-lang/rust-analyzer#15590

My unsafe-fu is not the best but it does satisfy miri.

There is still some follow up work to do, notably a lot of places using strings instead of symbols/names, most notably the token tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-perf performance issues C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) E-hard
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant