Implement symbol interning infra #17584
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
force-pushed
the
landing-page
branch
2 times, most recently
from
7ef7246 to
6643336
Compare
Veykril
force-pushed
the
landing-page
branch
3 times, most recently
from
8ac43e2 to
02e4561
Compare
crates/intern/src/symbol.rs
Outdated
| // SAFETY: The pointer is non-null as it is derived from a reference | ||
| // Ideally we would call out to `pack_arc` but for a `false` tag, unfortunately the | ||
| // packing stuff requires reading out the pointer to an integer which is not supported | ||
| // in const contexts, so here we make use of the fact that for the non-arc version the | ||
| // tag is false (0) and thus does not need touching the actual pointer value.ext) | ||
| packed: unsafe { | ||
| NonNull::new_unchecked((r as *const &str).cast::<*const str>().cast_mut()) | ||
| }, |
There was a problem hiding this comment.
I am not too fond of this part as I'd really like to make sure to clear out the LSB but I am not aware of a way of doing this that does not involve transmuting the address to a usize which is disallowed in const contexts
Veykril
changed the title
|
Oh dear I am getting a |
|
Just wondering, do we really need to support static strings, or is it more for the convenience of using |
|
Mostly the convenience I'd guess? Can't say how much of a perf difference it is in having the known symbols not participate in const re-allocation and ref counting |
|
I also just wanted to try myself at a bit of unsafe again as it has been a while :) Though I am quite confused as to why this is failing here. |
|
Okay found out where I was being stupid. Ugh, though the next problem is |
Veykril
force-pushed
the
landing-page
branch
2 times, most recently
from
70073af to
c18d25c
Compare
|
So this saves ~50mb of memory on |
|
This revealed a few places where we relying on iteration order of hashmaps in tests, which is now very much apparent given the hashes rely on pointer values. (also I just realized I forgot switch branches when creating the PR) |
|
@bors r+ |
|
Will tackle tt after #17559 to reduce conflicts |
|
☀️ Test successful - checks-actions |
| @@ -1348,6 +1348,7 @@ fn proc_attr(a: TokenStream, b: TokenStream) -> TokenStream { a } | |||
| .keys() | |||
| .map(|name| name.display(&db).to_string()) | |||
| .sorted() | |||
| .sorted() | |||
Will fix #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.