Use 128 bit instead of Symbol for crate disambiguator #45476

Xanewok · 2017-10-23T18:29:52Z

As discussed on gitter, this changes crate_disambiguator from Strings to what they are represented as, a 128 bit number.

There's also one bit I think also needs to change, but wasn't 100% sure how: create_root_def. Should I change DefKey::root_parent_stable_hash to accept Fingerprint as crate_disambiguator to quickly combine the hash of crate_name with the new 128 bit hash instead of a string for a disambiguator?

r? @michaelwoerister

EDIT: Are those 3 tests mir-opt failing, because the hash is different, because we calculate it a little bit differently (storing directly instead of hashing the hex-string representation)? Should it be updated like in #45319?

rust-highfive · 2017-10-23T18:30:05Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Xanewok · 2017-10-23T18:31:26Z

src/librustc_incremental/persist/fs.rs

    let crate_name = format!("{}-{}",
                             crate_name,
-                             base_n::encode(hasher.finish(), INT_ENCODE_BASE));
+                             base_n::encode(crate_disambiguator, INT_ENCODE_BASE));


This truncates the hash to 64 bits, should I implement a 128 but encoding in base_n?

This is pre-existing?

Now sure what you mean exactly, but to_smaller_hash() already exists and truncates to 64 bits.
I meant to ask if base_n::encode should be extended to work with Fingerprint/u128, in addition to u64. To be honest, I saw that it accepts u64, so thought that truncating 128->64 might be acceptable in this case

The code you wrote should have roughly the same effect as before since DefaultHasher also produces 64 bits. We don't want these hashes to be too long because they end up in directory names and long paths/names sometimes cause trouble for various reasons (file system support, operating system API restrictions, etc).

Maybe you could add a comment here, similar to the one that is replaced:

// The full crate disambiguator is really long. 64 bits of it should be // sufficient.

michaelwoerister

Thanks for the PR, @Xanewok! It already looks pretty good.

Browsing through the code though, I think it is a good idea to introduce a new type for the disambiguator, as in:

#[derive(Copy, Clone, Hash, Debug, ...)]
pub struct CrateDisambiguator {
    fingerprint: Fingerprint
}

impl CrateDisambiguator {
    pub fn to_fingerprint(self) -> Fingerprint {
        self.fingerprint
    }
}

You could put that into librustc/session/config.rs for example. It seems slightly cleaner to me if the disambiguator as its own type.

Regarding the mir-opt tests, yes, just update the hash values.

michaelwoerister · 2017-10-24T08:21:47Z

src/librustc/hir/map/collector.rs

@@ -118,7 +119,7 @@ impl<'a, 'hir> NodeCollector<'a, 'hir> {
    }

    pub(super) fn finalize_and_compute_crate_hash(self,
-                                                  crate_disambiguator: &str)
+                                                  crate_disambiguator: &Fingerprint)


Usually we pass around Fingerprint by value. It implements Copy so that should be easy.

michaelwoerister · 2017-10-24T08:30:25Z

src/librustc_incremental/persist/fs.rs

    let crate_name = format!("{}-{}",
                             crate_name,
-                             base_n::encode(hasher.finish(), INT_ENCODE_BASE));
+                             base_n::encode(crate_disambiguator, INT_ENCODE_BASE));


The code you wrote should have roughly the same effect as before since DefaultHasher also produces 64 bits. We don't want these hashes to be too long because they end up in directory names and long paths/names sometimes cause trouble for various reasons (file system support, operating system API restrictions, etc).

Maybe you could add a comment here, similar to the one that is replaced:

// The full crate disambiguator is really long. 64 bits of it should be // sufficient.

michaelwoerister · 2017-10-24T08:47:24Z

There's also one bit I think also needs to change, but wasn't 100% sure how: create_root_def. Should I change DefKey::root_parent_stable_hash to accept Fingerprint as crate_disambiguator to quickly combine the hash of crate_name with the new 128 bit hash instead of a string for a disambiguator?

Yes, you can do that. It would be a bit more consistent this way.

Xanewok · 2017-10-24T15:55:56Z

@michaelwoerister created a CrateDisambiguator newtype (awfully similar to DefPathHash) in session/mod.rs. config.rs had a lot of CLI/config-related data and logic and it somehow felt better to put this small type directly inside mod.rs, especially since it's used in Session - hope that's okay.
Addressed comments and fixed tests 🚀

michaelwoerister · 2017-10-25T09:14:42Z

Excellent, thanks a lot @Xanewok!

@bors r+

bors · 2017-10-25T09:14:43Z

📌 Commit 7fa64bc has been approved by michaelwoerister

bors · 2017-10-25T12:38:17Z

⌛ Testing commit 7fa64bc with merge f764eaf...

@michaelwoerister

…erister Use 128 bit instead of Symbol for crate disambiguator As discussed on gitter, this changes `crate_disambiguator` from Strings to what they are represented as, a 128 bit number. There's also one bit I think also needs to change, but wasn't 100% sure how: [create_root_def](https://github.com/rust-lang/rust/blob/f338dba29705e144bad8b2a675284538dd514896/src/librustc/hir/map/definitions.rs#L468-L482). Should I change `DefKey::root_parent_stable_hash` to accept `Fingerprint` as crate_disambiguator to quickly combine the hash of `crate_name` with the new 128 bit hash instead of a string for a disambiguator? r? @michaelwoerister EDIT: Are those 3 tests `mir-opt` failing, because the hash is different, because we calculate it a little bit differently (storing directly instead of hashing the hex-string representation)? Should it be updated like in #45319?

bors · 2017-10-25T15:33:58Z

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

michaelwoerister · 2017-10-26T08:59:48Z

🎉

Use 128 bit instead of Symbol for crate disambiguator

d017466

rust-highfive assigned michaelwoerister Oct 23, 2017

Xanewok commented Oct 23, 2017

View reviewed changes

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2017

michaelwoerister reviewed Oct 24, 2017

View reviewed changes

Introduce CrateDisambiguator newtype and fix tests

7fa64bc

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2017

michaelwoerister 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 Oct 25, 2017

bors merged commit 7fa64bc into rust-lang:master Oct 25, 2017

bors mentioned this pull request Oct 25, 2017

Avoid unnecessary copies of arguments that are simple bindings #45380

Merged

Xanewok mentioned this pull request Oct 27, 2017

Add crate disambiguators rust-dev-tools/rls-data#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 128 bit instead of Symbol for crate disambiguator #45476

Use 128 bit instead of Symbol for crate disambiguator #45476

Xanewok commented Oct 23, 2017 •

edited

Loading

rust-highfive commented Oct 23, 2017

Xanewok Oct 23, 2017

arielb1 Oct 23, 2017

Xanewok Oct 23, 2017

michaelwoerister Oct 24, 2017

michaelwoerister left a comment

michaelwoerister Oct 24, 2017

michaelwoerister Oct 24, 2017

michaelwoerister commented Oct 24, 2017

Xanewok commented Oct 24, 2017

michaelwoerister commented Oct 25, 2017

bors commented Oct 25, 2017

bors commented Oct 25, 2017

bors commented Oct 25, 2017

michaelwoerister commented Oct 26, 2017

Use 128 bit instead of Symbol for crate disambiguator #45476

Use 128 bit instead of Symbol for crate disambiguator #45476

Conversation

Xanewok commented Oct 23, 2017 • edited Loading

rust-highfive commented Oct 23, 2017

Xanewok Oct 23, 2017

Choose a reason for hiding this comment

arielb1 Oct 23, 2017

Choose a reason for hiding this comment

Xanewok Oct 23, 2017

Choose a reason for hiding this comment

michaelwoerister Oct 24, 2017

Choose a reason for hiding this comment

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Oct 24, 2017

Choose a reason for hiding this comment

michaelwoerister Oct 24, 2017

Choose a reason for hiding this comment

michaelwoerister commented Oct 24, 2017

Xanewok commented Oct 24, 2017

michaelwoerister commented Oct 25, 2017

bors commented Oct 25, 2017

bors commented Oct 25, 2017

bors commented Oct 25, 2017

michaelwoerister commented Oct 26, 2017

Xanewok commented Oct 23, 2017 •

edited

Loading