Skip to content
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

TypeId should be (u64, u64) instead of u128 to save space #115620

Closed
stepancheg opened this issue Sep 6, 2023 · 6 comments · Fixed by #121358
Closed

TypeId should be (u64, u64) instead of u128 to save space #115620

stepancheg opened this issue Sep 6, 2023 · 6 comments · Fixed by #121358
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@stepancheg
Copy link
Contributor

stepancheg commented Sep 6, 2023

On Apple Silicon (unlike x86_64) u128 is 16-bytes aligned. So it takes extra space with little or no benefit. I looked at generated code, it is the same:

https://rust.godbolt.org/z/3sE7sMsjj

(Note just using repr(packed(8)) won't work because it will pass TypeId by reference not by value)

Additionally the issue with current TypeId is that is has different alignment on different platforms. Rust does not provide guarantees about the alignment, but if this was the same everywhere, it'd make life easier. (Practically we encountered this issue when our CI worked fine on Apple x86_64 macs, but the program didn't work on Apple Silicon.)

rustc 1.74.0-nightly (a991861 2023-09-05)
macOS, Apple Silicon

@stepancheg stepancheg added the C-bug Category: This is a bug. label Sep 6, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 6, 2023
@Noratrieb Noratrieb added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 7, 2023
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2023

Note that (u64, u64) doesn't have the same alignment on all platforms either. It will be 4-aligned on (some) 32bit platforms.

@cactusdualcore
Copy link

cactusdualcore commented Dec 5, 2023

Note just using repr(packed(8)) won't work because it will pass TypeId by reference not by value

I might be wrong here, but

let x = 1u128;
{
    let y = x;
    println!("{}", y);
}
println!("{}", x);

seems to work, so I believe u128 is Copy. So can't said problem be eliminated by implementing Copy on TypeId as well?

@stepancheg
Copy link
Contributor Author

@Cactus-Man TypeId already implements Copy. Also Copy does not affect alignment.

@lygstate
Copy link
Contributor

Note that (u64, u64) doesn't have the same alignment on all platforms either. It will be 4-aligned on (some) 32bit platforms.

If that's the case, how about (u32,u32,u32,u32)

@stepancheg
Copy link
Contributor Author

If that's the case, how about (u32,u32,u32,u32)

That would make hashing and probably equality slower.

@lygstate
Copy link
Contributor

If that's the case, how about (u32,u32,u32,u32)

That would make hashing and probably equality slower.

on 32 bit system would not be slower

@bors bors closed this as completed in a1f6191 Mar 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 9, 2024
Rollup merge of rust-lang#121358 - GnomedDev:lower-align-typeid, r=Mark-Simulacrum

Reduce alignment of TypeId to u64 alignment

Closes rust-lang#115620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants