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

during incr. comp. place .o files in incr. comp. directory and give predictable name #34036

Closed
nikomatsakis opened this issue Jun 2, 2016 · 17 comments
Labels
A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

As part of the incr. comp. work, we need to place the intermediate .o files for each partition into the incr. comp. directory (and not erase them after we're done). They should be given names that reflect the name of the partition they represent. This will let us reuse them in later compilations when possible.

cc @michaelwoerister

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-incr-comp Area: Incremental compilation labels Jun 2, 2016
@nagisa
Copy link
Member

nagisa commented Jun 2, 2016

Seems to me like using the hash generated as part of #34037 should suffice? Not only, then, it would be trivial to check for need to regenerate only the necessary files (doesn’t exist? rebuild), but also easy to keep clean (didn’t use that hash? remove.)

@nikomatsakis
Copy link
Contributor Author

@nagisa yeah, we could use the hashes, though the partitions already have stable names at present, so that works too.

@michaelwoerister
Copy link
Member

I'd opt for including a human-readable name into the obj file name for debugging reasons.

@nikomatsakis
Copy link
Contributor Author

OK, so, I made a branch that places .o files (and other temporary artifacts) into the incremental compilation directory. It also opts not to delete .o files when the work is done (at least those corresponding to modules). I was debating though whether it'd be better to instead copy the files into the incremental directory when the work is done (and then copy them back out again if we want to re-use them). The idea would be that the "external interface" of the compiler remains the same -- if you run in incremental mode, and you pass --save-temps (or whatever we call it now), you still wind up with your temporary files in the output directory (whereas in my branch they will be in the incremental directory instead). The cost is that we have to copy some files.

@nikomatsakis
Copy link
Contributor Author

I will open the PR once #33890 lands.

@nikomatsakis
Copy link
Contributor Author

cc @rust-lang/tools curious to get your take on this comment

@eddyb
Copy link
Member

eddyb commented Jun 4, 2016

I believe that in the long term, in a situation without dynamic libraries, we would want to avoid generating intermediary rlibs (at least Cargo doesn't need them) and always link together .o files directly into the final build product, and this is a good first step towards that.

@michaelwoerister
Copy link
Member

I made a branch that places .o files (and other temporary artifacts) into the incremental compilation directory.

What does it do when no incremental compilation directory is given? Just use the output directory?

@nikomatsakis
Copy link
Contributor Author

@michaelwoerister yes, as before.

@alexcrichton
Copy link
Member

It also opts not to delete .o files when the work is done (at least those corresponding to modules).

If you run in incremental mode, how would this work if we deleted the object files? Don't we have to leave them around to reuse them?

I do think though that rustc foo.rs should still not litter the current directory, but rustc foo.rs --incremental bar should be able to do whatever it wants to the directory bar.

I was debating though whether it'd be better to instead copy the files into the incremental directory when the work is done (and then copy them back out again if we want to re-use them).

Oh so we always leave it in the incremental directory, and only maybe move it out? -C save-temps isn't really specified so I'd be ok if we didn't actively move it out, but if we normally leave it in the incremental dir this seems fine.

The cost is that we have to copy some files.

I've found a great way to handle this is:

fs::hard_link(src, dst).or_else(|_| fs::copy(src, dst))

That way it attempts to do the super fast thing first which normally succeeds, but in weird cases it'll copy some files.

@nikomatsakis
Copy link
Contributor Author

@alexcrichton I can't really tell which design you are advocating for :) but yes that hard-link thing seems elegant. Maybe it's better then to keep everything in the output directory (as today) but also copy/link things into or out of the incr. dir in incremental mode. Obviously not a huge deal either way.

@alexcrichton
Copy link
Member

Oh hm I may just actually be confused about what the possible designs here are. To me the title of this issue is an clear "we can literally never be incremental without doing that". The next piece:

It also opts not to delete .o files when the work is done (at least those corresponding to modules).

So right now we spray a bunch of files in the output directory, and do you mean that we never delete these files if incremental is turned on? If that's the case then I'd instead recommend what I think your next strategy was with always emit object files into the incremental directory directly, and then only if -C save-temps is passed are they copied out (although that's somewhat optional in my opinion).

Does that make sense?

@michaelwoerister
Copy link
Member

If that's the case then I'd instead recommend what I think your next strategy was with always emit object files into the incremental directory directly, and then only if -C save-temps is passed are they copied out (although that's somewhat optional in my opinion).

This is the option that makes the most sense to me.

@nikomatsakis
Copy link
Contributor Author

Hmm. That is indeed a "third way". It's the most complicated to implement, but perhaps the best. Basically we'll generate all files into the "temporary directory" (which is the incremental directory or, if not in incr. mode, the output directory).

When done, what we do depends on our mode:

  • non-incremental-mode: delete files unless save-temps is on
  • incremental-mode: copy files if save-temp is on, then delete everything but the .o files

I'll see if I can make this clean in the code =)

@alexcrichton
Copy link
Member

Note that I also don't mind simplifying it and saying that if we're in incremental mode and save-temps is on we just do nothing. I don't think -C save-temps is heavily relied on anywhere and unless you know what the compiler is doing already it's not too useful.

For example we've changed object file names a bunch over time so there's never been a way to in a stable method leverage the flag beyond one-off debugging.

@nikomatsakis
Copy link
Contributor Author

I am now leaning towards my original plan -- keep everything as it is, but link/copy files out of the output directory and into the incremental directory. The reason is that the filenames we currently generate include the user's choice of "filestem" -- so, for example, if they are running like this:

rustc -C incremental=$I -o crate1/lib.a ...
rustc -C incremental=$I -o crate2/lib.a ...

we will generate files like crate1/lib.xxx.o and so forth. Under my patch, those files would be named $I/lib.xxx.o, but that still seems suboptimal. I would prefer that we have total control over the file names that are used in the incremental directory, so that when we do cleanups (like deleting random .o files that belong to partitions that no longer exist) we can expect filenames to fit a particular schema.

I guess one option is just to further alter the OutputNames code so that, for temporary files, it ignores the user's filestem and always uses the crate-disambiguator pattern (which, frankly, makes some sense).

But overall it seems easier to do the following:

  1. Compile as normal, using output directory.
    • If in incremental mode, and a given CGU is still "fresh" (in cargo terms), link/copy the .o file from the incremental directory into the output directory, using whatever name we would normally use.
  2. If incremental mode is given, link/copy any .o files into the incremental directory with the name we want.
  3. Unless save-temps mode is given, erase temporary files.

As a bonus, save-temps works as it does today (and as I think we probably want), though I agree that's not a particularly big deal.

I just want to drill a bit more into the downsides of this plan, I guess. The main thing I can see is that we have to link/copy even in the normal path -- though I wouldn't really expect that to be expensive except maybe in extreme situations. Are there other problems?

@alexcrichton
Copy link
Member

I would prefer that we have total control over the file names that are used in the incremental directory

This is a very good point! I agree that we should always have it such that the format of the incremental directory is entirely up to us and isn't stable at all.

I think your plan sounds good to me, the only perf case that really matters is when you need to reload a file from that directory, but when we link executables today we're already throwing around rlibs on the filesystem and in 99% of cases a hard link will work so I wouldn't be too worried about the perf (mainly b/c of hard links).

@nikomatsakis nikomatsakis added this to the Incremental compilation alpha milestone Jul 23, 2016
bors added a commit that referenced this issue Jul 28, 2016
Enable reuse of `.o` files if nothing has changed

This PR completes a first "spike" for incremental compilation by enabling us to reuse `.o` files when nothing has changed. When in incr. mode, we will save `.o` files into the temporary directory, then copy them back out again if they are still valid. The code is still a bit rough but it does seem to work. =)

r? @michaelwoerister

Fixes #34036
Fixes #34037
Fixes #34038
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 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