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

[incremental] Specialize encoding and decoding of Fingerprints #47243

Merged
merged 2 commits into from
Jan 11, 2018

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Jan 6, 2018

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

Copy link
Member

@michaelwoerister michaelwoerister left a 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.

@@ -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)) };
Copy link
Member

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...


let (l, r): (u64, u64) = unsafe { mem::transmute(bytes) };

Ok(Fingerprint(l, r))
Copy link
Member

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;)

@wesleywiser
Copy link
Member Author

@michaelwoerister fixed

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2018
@michaelwoerister
Copy link
Member

Thanks, @wesleywiser! Looks great now.

@eddyb, do you know if rustc could potentially re-order the fields of the tuple in memory and cause the transmute to be problematic here:
https://github.com/rust-lang/rust/pull/47243/files#diff-4041379a1583f33e58f7f2cd2aafc80aR53

Or are tuples always #[repr(C)]?

@eddyb
Copy link
Member

eddyb commented Jan 9, 2018

@michaelwoerister They're not #[repr(C)], use an array or a #[repr(C)] struct instead.

@michaelwoerister
Copy link
Member

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])))

@eddyb
Copy link
Member

eddyb commented Jan 9, 2018

slices cannot be destructured unfortunately

You can do let [a, b]: [u64; 2] - you're in the compiler, it's just a feature gate away.

@michaelwoerister
Copy link
Member

The #![feature(slice_patterns)] is already enabled for this crate, so you can use destructuring as @eddyb suggests.

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
@wesleywiser
Copy link
Member Author

Fixed

@michaelwoerister
Copy link
Member

Awesome, thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 10, 2018

📌 Commit 01c890e has been approved by michaelwoerister

@rust-lang rust-lang deleted a comment from bors Jan 10, 2018
@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2018
@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

@bors p=15

Just moving everything which may improve performance up 😭

bors added a commit that referenced this pull request Jan 11, 2018
…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
@bors
Copy link
Contributor

bors commented Jan 11, 2018

⌛ Testing commit 01c890e with merge c9c2980...

@bors
Copy link
Contributor

bors commented Jan 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing c9c2980 to master...

@bors bors merged commit 01c890e into rust-lang:master Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants