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

Speed up SVH computation by using Fingerprint::combine(). #47297

Closed
michaelwoerister opened this issue Jan 9, 2018 · 7 comments
Closed

Speed up SVH computation by using Fingerprint::combine(). #47297

michaelwoerister opened this issue Jan 9, 2018 · 7 comments
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@michaelwoerister
Copy link
Member

Existing fingerprint values can be merged into one fingerprint via the rather efficient Fingerprint::combine() method. We should make use of that during SVH computation instead of re-hashing all those fingerprints.

See here:

pub(super) fn finalize_and_compute_crate_hash(self,
crate_disambiguator: CrateDisambiguator,
cstore: &CrateStore,
commandline_args_hash: u64)
-> (Vec<MapEntry<'hir>>, Svh) {
let mut node_hashes: Vec<_> = self
.hir_body_nodes
.iter()
.map(|&(def_path_hash, dep_node_index)| {
(def_path_hash, self.dep_graph.fingerprint_of(dep_node_index))
})
.collect();
node_hashes.sort_unstable_by(|&(ref d1, _), &(ref d2, _)| d1.cmp(d2));
let mut upstream_crates: Vec<_> = cstore.crates_untracked().iter().map(|&cnum| {
let name = cstore.crate_name_untracked(cnum).as_str();
let disambiguator = cstore.crate_disambiguator_untracked(cnum)
.to_fingerprint();
let hash = cstore.crate_hash_untracked(cnum);
(name, disambiguator, hash)
}).collect();
upstream_crates.sort_unstable_by(|&(name1, dis1, _), &(name2, dis2, _)| {
(name1, dis1).cmp(&(name2, dis2))
});
let (_, crate_dep_node_index) = self
.dep_graph
.with_task(DepNode::new_no_params(DepKind::Krate),
&self.hcx,
((node_hashes, upstream_crates),
(commandline_args_hash,
crate_disambiguator.to_fingerprint())),
identity_fn);
let svh = Svh::new(self.dep_graph
.fingerprint_of(crate_dep_node_index)
.to_smaller_hash());
(self.map, svh)
}

@kennytm kennytm added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-incr-comp Area: Incremental compilation labels Jan 9, 2018
@michaelwoerister michaelwoerister added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 10, 2018
@srinivasreddy
Copy link
Contributor

I am taking it.

@michaelwoerister
Copy link
Member Author

Thank you, @srinivasreddy! Let me know if you need any further directions.

@Pulkit07
Copy link
Contributor

@srinivasreddy are you still working on it or can I take this one?

@michaelwoerister
Copy link
Member Author

Feel free to give it a shot if you are still interested, @Pulkit07!

@hrvolapeter
Copy link
Member

hrvolapeter commented Feb 23, 2018

Hello @michaelwoerister I would like to take this one, but first I will need a bit more explanation since I'm new to this project.

You are suggestings using Fingerprint::combine() method I suppose you meant instead of this

.map(|&(def_path_hash, dep_node_index)| { 
             (def_path_hash, self.dep_graph.fingerprint_of(dep_node_index)) 
         }) 

create just one fingerprint.

There is a commit that is doing what I described, but I'm not sure if it's fine https://github.com/retep007/rust/commit/0c3bf9d82c1dd8c7304ac241bc610efc85ab87fc , particularly if merging vector of hashes to one can't affect something else

@hrvolapeter
Copy link
Member

@michaelwoerister did you have time to check my commit?

@michaelwoerister
Copy link
Member Author

Oh, sorry for the delay, @retep007! I remember taking a look and then completely forgot replying to you.

The code combining the fingerprints looks good. However, we have to keep the sorting step in order to make the hash more stable. Otherwise exchanging to functions in the source code would lead to a different hash, even though such a change makes no semantic difference.

kennytm added a commit to kennytm/rust that referenced this issue Mar 14, 2018
…on, r=michaelwoerister

Speed up SVH computation by using Fingerprint::combine()

Fix rust-lang#47297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants