Save/load incremental compilation dep graph #32016

nikomatsakis · 2016-03-02T23:15:18Z

Contains the code to serialize/deserialize the dep graph to disk between executions. We also hash the item contents and compare to the new hashes. Also includes a unit test harness. There are definitely some known limitations, such as #32014 and #32015, but I am leaving those for follow-up work.

Note that this PR builds on #32007, so the overlapping commits can be excluded from review.

r? @michaelwoerister

nikomatsakis · 2016-03-02T23:34:25Z

Ah, I forgot about the impl variant thing. We may not want to merge this just yet until 61202ad is solved in some better way, since people will complain about their symbol names. :)

alexcrichton · 2016-03-03T00:46:17Z

📫

nikomatsakis · 2016-03-03T21:31:11Z

Doing a quick rebase now.

nikomatsakis · 2016-03-03T21:39:10Z

@michaelwoerister argh -- I forgot when we were just talking about the "symbol names for impl" business. I should probably fix that before we land this PR. Still not sure of the best way to do so though, I'll have to think about it. Maybe I can do some quick hack so things are "no worse" than today.

michaelwoerister · 2016-03-05T21:34:43Z

I've taken a look at the Itanium C++ ABI symbol mangling scheme (which GCC uses) and it does not seem to support something like <Type as Trait>::method. It might be easier to do Trait<SelfType, T1, T2>::method. I have not done any concrete experiments on what tools actually support yet though...

bors · 2016-03-06T09:16:56Z

☔ The latest upstream changes (presumably #30884) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-03-07T10:36:54Z

Sigh. I wish they had just used something generic like URL-encoding.

nikomatsakis · 2016-03-08T14:49:01Z

@michaelwoerister

The comments above say that revision have to start with "pass" or "fail" instead of "rpass" or "cfail", etc. Or is this something different?

Just out of date, I think.

michaelwoerister · 2016-03-08T16:21:27Z

I've done a first pass over the PR. In general it looks good to me. The incremental test framework looks pretty neat. But I have to do another, more in-depth, reading of the code, especially the dep-graph serialization.
But it seems that there's still quite a few smaller things to do before this passes make check anyway.

One thing that I noticed was that there are some changes that show up one or two commits early, so that those commits won't compile. Do we have some policy on this? Should each commit be valid on its own?

nikomatsakis · 2016-03-08T17:18:56Z

One thing that I noticed was that there are some changes that show up one or two commits early, so that those commits won't compile. Do we have some policy on this? Should each commit be valid on its own?

AFAIK, we don't, and they need not be. We can still easily bisect using the bors merge commits. I just tried to break it up into logical steps for your convenience -- but I don't think I'll go back and make fake history just to ensure that everything builds at each step (the real history was ... a bit convoluted).

bors · 2016-03-09T03:25:20Z

☔ The latest upstream changes (presumably #32126) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-03-14T11:39:32Z

@michaelwoerister so I'm rebasing this atop your other PR and making various adjustments. Will ping you when ready.

bors · 2016-03-27T05:03:31Z

☔ The latest upstream changes (presumably #32513) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2016-03-28T23:20:19Z

ping @michaelwoerister rebased and ready for re-review :) -- as before the commits are not truly standalone, but hopefully broken up enough to give you some sense of the major pieces

michaelwoerister · 2016-03-28T23:22:21Z

Thanks for the notification. I try to do the review this week.

bors · 2016-03-30T04:25:53Z

☔ The latest upstream changes (presumably #32576) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · 2016-04-04T13:54:35Z

Going to take a look at this today...

michaelwoerister · 2016-04-04T14:40:23Z

src/librustc_incremental/calculate_svh.rs


        "crate_disambiguator".hash(&mut state);
-        crate_disambiguator.len().hash(&mut state);
-        crate_disambiguator.hash(&mut state);
+        crate_disambiguator.as_str().len().hash(&mut state);


We might get into trouble here with endianess.

@michaelwoerister I guess we should discuss the role of the SVH. I see it as being used for two purposes:

to drive incr. comp.; for this role, endian-ness is no problem

to check for duplicate versions of a crate; here it's a bit less clear, endianness could be an issue in the sense that compiling the same source could produce distinct hashes on different host architectures.

I am somewhat...surprised to see that write_usize for hashes does seem to convert to a [u8; 4]. Crazy.

I guess we can write a "endian-proof" wrapper, though there is also code which converts &[usize] to &[u8], which would be harder.

I'm not sure if it's a good idea to re-use the SVH for incremental compilation. It's something with a specific purpose (2) and trying to make it hit two goals at once might just make it more complicated. The other way round though, re-using a well-defined incremental compilation hash when computing the SVH makes more sense to me.

ad endianess: Yes, for incremental compilation it doesn't make a difference. For the other case: I guess you can never have two crates with differing endianess in the your crate graph. So, in the end it should not matter in either case. It's more of code smell than a bug anyway.

On Mon, Apr 04, 2016 at 08:48:39AM -0700, Michael Woerister wrote:

"crate_disambiguator".hash(&mut state);

crate_disambiguator.len().hash(&mut state);

crate_disambiguator.hash(&mut state);

crate_disambiguator.as_str().len().hash(&mut state);

I'm not sure if it's a good idea to re-use the SVH for incremental compilation. It's something with a specific purpose (2) and trying to make it hit two goals at once might just make it more complicated. The other way round though, re-using a well-defined incremental compilation hash when computing the SVH makes more sense to me.

I agree that the SVH as originally envisioned is not what we want --
for one thing, it tries to have some (broken) notion of not hashing
things that do not affect "ABI compatibilty". I think there is no use
for the existing SVH. I am basically taking it over and repurposing
it. I should rename it to "ICH" (incr. comp. hash), probably. But the
current names seems also not inappropriate.

Do you think that it's worth retaining the "SVH as it exists today"?

ad endianess: Yes, for incremental compilation it doesn't make a
difference. For the other case: I guess you can never have two
crates with differing endianess in the your crate graph. So, in the
end it should not matter in either case. It's more of code smell
than a bug anyway.

well, what I was concerned about is that you cross-compile the library
on one machine. then copy it somewhere else. then recompile something
it depends on on that local machine. now if the endianness's differ
rustc may get whiny, even though same toolchain/version/everything was
used, because the SVH recorded in the (cross-compiled) crate is
different from the one that is found in the (locally) compiled dep.
Admittedly this scenario seems a bit far fetched and like something
that can be relegated to a FIXME.

Niko

what I was concerned about is that you cross-compile the library

That's true. I think, I would just fix it with something like: (crate_disambiguator.as_str().len() as u64).to_le().hash(...)

Hmm, ok

On Mon, Apr 04, 2016 at 10:00:47AM -0700, Michael Woerister wrote:

"crate_disambiguator".hash(&mut state);

crate_disambiguator.len().hash(&mut state);

crate_disambiguator.hash(&mut state);

crate_disambiguator.as_str().len().hash(&mut state);

what I was concerned about is that you cross-compile the library

That's true. I think, I would just fix it with something like: (crate_disambiguator.as_str().len() as u64).to_le().hash(...)

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/rust-lang/rust/pull/32016/files/601f8c66256df0d4d1fe2ec3d3f617a6fae08f68..aa5eca284c2e220cac461a5626a559690f664fe6#r58409485

Although I admit that's one ugly line of code that really asks to be hidden behind some kind of utility function at some point.

Not sure what was the bug. Fixes rust-lang#32014.

nikomatsakis · 2016-04-06T20:42:41Z

@bors r=michaelwoerister

bors · 2016-04-06T20:42:41Z

📌 Commit 54d78a4 has been approved by michaelwoerister

bors · 2016-04-06T23:43:58Z

⌛ Testing commit 54d78a4 with merge fd7614e...

bors · 2016-04-07T00:11:43Z

💔 Test failed - auto-linux-64-opt-rustbuild

nikomatsakis · 2016-04-07T00:21:16Z

@bors r=mw

bors · 2016-04-07T00:21:16Z

📌 Commit 29ad9a2 has been approved by mw

bors · 2016-04-07T03:11:14Z

⌛ Testing commit 29ad9a2 with merge 89fbd29...

bors · 2016-04-07T03:51:42Z

💔 Test failed - auto-linux-64-x-freebsd

Rollup of 11 pull requests - Successful merges: #32016, #32583, #32699, #32729, #32731, #32738, #32741, #32745, #32748, #32757, #32786 - Failed merges: #32773

nikomatsakis · 2016-04-07T17:09:28Z

@bors r=mw

bors · 2016-04-07T17:09:29Z

📌 Commit 9eaae92 has been approved by mw

bors · 2016-04-07T17:55:37Z

⌛ Testing commit 9eaae92 with merge 7979dd6...

@michaelwoerister

Save/load incremental compilation dep graph Contains the code to serialize/deserialize the dep graph to disk between executions. We also hash the item contents and compare to the new hashes. Also includes a unit test harness. There are definitely some known limitations, such as #32014 and #32015, but I am leaving those for follow-up work. Note that this PR builds on #32007, so the overlapping commits can be excluded from review. r? @michaelwoerister

alexcrichton · 2016-04-07T22:40:32Z

The android bot disappeared, but all other tests pass, so I'm gonna merge manually.

Praying this actually works for android :)

rust-highfive assigned michaelwoerister Mar 2, 2016

nikomatsakis force-pushed the incr-comp-save branch from c1295ad to aa99e5d Compare March 3, 2016 21:38

nikomatsakis force-pushed the incr-comp-save branch from 6bd48ec to 92729d4 Compare March 8, 2016 17:23

nikomatsakis mentioned this pull request Mar 10, 2016

WIP: Implement stable symbol-name generation algorithm. #31539

Closed

nikomatsakis force-pushed the incr-comp-save branch 2 times, most recently from 59cff17 to 14023bb Compare March 19, 2016 00:03

nikomatsakis force-pushed the incr-comp-save branch 2 times, most recently from ed9b2ad to cf8c869 Compare March 25, 2016 21:14

nikomatsakis force-pushed the incr-comp-save branch 2 times, most recently from 38eae85 to 4a0e984 Compare March 28, 2016 23:19

michaelwoerister reviewed Apr 4, 2016
View reviewed changes

rebase over the hir rename

22b4bb0

nikomatsakis force-pushed the incr-comp-save branch from 7d6e369 to 22b4bb0 Compare April 6, 2016 18:05

Fix test cases to reflect that rust-lang#32014...went away

54d78a4

Not sure what was the bug. Fixes rust-lang#32014.

remove incorrect dependency on librustc_front

29ad9a2

Manishearth mentioned this pull request Apr 7, 2016

Rollup of 11 pull requests #32787

Closed

bors added a commit that referenced this pull request Apr 7, 2016

Auto merge of #32787 - Manishearth:rollup, r=Manishearth

6a55f4f

Rollup of 11 pull requests - Successful merges: #32016, #32583, #32699, #32729, #32731, #32738, #32741, #32745, #32748, #32757, #32786 - Failed merges: #32773

argh, overlooked two extern crates

9eaae92

alexcrichton merged commit 9eaae92 into rust-lang:master Apr 7, 2016

This was referenced Apr 7, 2016

Feature gate clean #32791

Merged

change hir::ExprLit to take a ConstVal instead of an ast::Lit #32793

Closed

rustbuild: Add support for compiletest test suites #32755

Merged

michaelwoerister mentioned this pull request Jul 19, 2016

Clarify cross-crate dependency story #32015

Closed

nikomatsakis deleted the incr-comp-save branch October 3, 2016 14:55

Save/load incremental compilation dep graph #32016

Save/load incremental compilation dep graph #32016

Conversation

nikomatsakis commented Mar 2, 2016

nikomatsakis commented Mar 2, 2016

alexcrichton commented Mar 3, 2016

nikomatsakis commented Mar 3, 2016

nikomatsakis commented Mar 3, 2016

michaelwoerister commented Mar 5, 2016

bors commented Mar 6, 2016

nikomatsakis commented Mar 7, 2016

nikomatsakis commented Mar 8, 2016

michaelwoerister commented Mar 8, 2016

nikomatsakis commented Mar 8, 2016

bors commented Mar 9, 2016

nikomatsakis commented Mar 14, 2016

bors commented Mar 27, 2016

nikomatsakis commented Mar 28, 2016

michaelwoerister commented Mar 28, 2016

bors commented Mar 30, 2016

michaelwoerister commented Apr 4, 2016

michaelwoerister Apr 4, 2016

Choose a reason for hiding this comment

nikomatsakis Apr 4, 2016

Choose a reason for hiding this comment

michaelwoerister Apr 4, 2016

Choose a reason for hiding this comment

nikomatsakis Apr 4, 2016

Choose a reason for hiding this comment

michaelwoerister Apr 4, 2016

Choose a reason for hiding this comment

nikomatsakis Apr 4, 2016

Choose a reason for hiding this comment

michaelwoerister Apr 4, 2016

Choose a reason for hiding this comment

nikomatsakis commented Apr 6, 2016

bors commented Apr 6, 2016

bors commented Apr 6, 2016

bors commented Apr 7, 2016

nikomatsakis commented Apr 7, 2016

bors commented Apr 7, 2016

bors commented Apr 7, 2016

bors commented Apr 7, 2016

nikomatsakis commented Apr 7, 2016

bors commented Apr 7, 2016

bors commented Apr 7, 2016

alexcrichton commented Apr 7, 2016