-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[incremental] Specialize encoding and decoding of Fingerprints #47243
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
Conversation
1f02e4f
to
8b39266
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @wesleywiser! This looks very good. Looking forward to seeing if it also has an impact on compile times.
I added some comments below: The encoding still has to be made platform-indepedent but that should be easy to implement.
src/librustc/ich/fingerprint.rs
Outdated
@@ -46,6 +49,21 @@ impl Fingerprint { | |||
format!("{:x}{:x}", self.0, self.1) | |||
} | |||
|
|||
pub fn encode_opaque(&self, encoder: &mut Encoder) -> EncodeResult { | |||
let bytes: [u8; 16] = unsafe { mem::transmute((self.0, self.1)) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are emitting raw bytes here we can't rely on leb128 to take care integer endianess, so we have to do it ourselves. Fortunately that should be straightforward. Just write the following here:
let bytes: [u8; 16] = unsafe { mem::transmute((self.0.to_le(), self.1.to_le())) };
and...
src/librustc/ich/fingerprint.rs
Outdated
|
||
let (l, r): (u64, u64) = unsafe { mem::transmute(bytes) }; | ||
|
||
Ok(Fingerprint(l, r)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... the following here:
Ok(Fingerprint(u64::from_le(l), u64::from_le(r)))
(If you get a method resolution error, you might have to use std::u64;
)
8b39266
to
7c1c2be
Compare
@michaelwoerister fixed |
Thanks, @wesleywiser! Looks great now. @eddyb, do you know if Or are tuples always |
@michaelwoerister They're not |
Thanks for the quick reply, @eddyb! @wesleywiser, the following should avoid running into memory layout problems: ...
// When encoding
let bytes: [u8; 16] = unsafe { mem::transmute([self.0.to_le(), self.1.to_le()]) };
...
// When decoding (slices cannot be destructured unfortunately).
let fingerprint: [u64; 2] = unsafe { mem::transmute(bytes) };
Ok(Fingerprint(u64::from_le(fingerprint[0]), u64::from_le(fingerprint[1]))) |
You can do |
The |
This saves the storage space used by about 32 bits per `Fingerprint`. On average, this reduces the size of the `/target/{mode}/incremental` folder by roughly 5%. Fixes rust-lang#45875
7c1c2be
to
01c890e
Compare
Fixed |
Awesome, thank you! @bors r+ |
📌 Commit 01c890e has been approved by |
@bors p=15 Just moving everything which may improve performance up 😭 |
…elwoerister [incremental] Specialize encoding and decoding of Fingerprints This saves the storage space used by about 32 bits per `Fingerprint`. On average, this reduces the size of the `/target/{mode}/incremental` folder by roughly 5% [Full details here](https://gist.github.com/wesleywiser/264076314794fbd6a4c110d7c1adc43e). Fixes #45875 r? @michaelwoerister
☀️ Test successful - status-appveyor, status-travis |
This saves the storage space used by about 32 bits per
Fingerprint
.On average, this reduces the size of the
/target/{mode}/incremental
folder by roughly 5% Full details here.
Fixes #45875
r? @michaelwoerister