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

rustc: Move bytecode compression into codegen #45399

Merged
merged 1 commit into from
Oct 22, 2017

Conversation

alexcrichton
Copy link
Member

This commit moves compression of the bytecode from the link module to the
write module, namely allowing it to be (a) cached by incremental compilation
and (b) produced in parallel. The parallelization may show up as some nice wins
during normal compilation and the caching in incremental mode should be
beneficial for incremental compiles! (no more need to recompress the entire
crate's bitcode on all builds)

@rust-highfive
Copy link
Contributor

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @michaelwoerister

I'm sort of perpetually unhappy with how no matter what I do the management of filenames in the backend is confusing, but hopefully it's a little less confusing than before?

@kennytm 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 20, 2017
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.

Great find! The new parallelization and caching should definitely be a win here.

The only downside I can see is that this will increase the size of the incr. cache. But hopefully we can get rid of bitcode compression entirely at some point by switching to MIR-only RLIBs.

r=me modulo nits and fixing the test case.

@@ -1157,17 +1183,20 @@ fn execute_work_item(cgcx: &CodegenContext,
err));
}
}
*slot = Some(obj_out);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you do this already in the match arms above?

@alexcrichton
Copy link
Member Author

@michaelwoerister

The only downside I can see is that this will increase the size of the incr. cache

Oh actually we're no longer storing uncompressed bytecode in the cache (as that's not needed for the linking stage), but default builds (those without fancy --emit flags) switch from storing object/bytecode to object/compressed bytecode

@michaelwoerister
Copy link
Member

Oh actually we're no longer storing uncompressed bytecode in the cache

We'll probably have to start doing that for ThinLTO, right?

@alexcrichton
Copy link
Member Author

Heh I've thought very little about how ThinLTO interacts with incremental compilation :)

In theory there's a lot of possible wins to gain there (fully incremental release builds) but I think we've got a good bit of support on the LLVM side of things to sort out before that works.

@alexcrichton
Copy link
Member Author

@bors: r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Oct 20, 2017

📌 Commit 3bfa312 has been approved by michaelwoerister

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 20, 2017
@alexcrichton alexcrichton force-pushed the compress-parallel branch 2 times, most recently from 2effe64 to 9963d13 Compare October 20, 2017 18:46
@alexcrichton
Copy link
Member Author

@bors: r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Oct 20, 2017

📌 Commit 9963d13 has been approved by michaelwoerister

@bors
Copy link
Collaborator

bors commented Oct 21, 2017

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

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 21, 2017
This commit moves compression of the bytecode from the `link` module to the
`write` module, namely allowing it to be (a) cached by incremental compilation
and (b) produced in parallel. The parallelization may show up as some nice wins
during normal compilation and the caching in incremental mode should be
beneficial for incremental compiles! (no more need to recompress the entire
crate's bitcode on all builds)
@alexcrichton
Copy link
Member Author

@bors: r=michaelwoerister

@bors
Copy link
Collaborator

bors commented Oct 21, 2017

📌 Commit 8197a0b has been approved by michaelwoerister

bors added a commit that referenced this pull request Oct 21, 2017
…ster

rustc: Move bytecode compression into codegen

This commit moves compression of the bytecode from the `link` module to the
`write` module, namely allowing it to be (a) cached by incremental compilation
and (b) produced in parallel. The parallelization may show up as some nice wins
during normal compilation and the caching in incremental mode should be
beneficial for incremental compiles! (no more need to recompress the entire
crate's bitcode on all builds)
@bors
Copy link
Collaborator

bors commented Oct 22, 2017

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

@bors bors merged commit 8197a0b into rust-lang:master Oct 22, 2017
@michaelwoerister
Copy link
Member

This seems to have brought some nice performance improvements: http://perf.rust-lang.org/compare.html?start=4279e2b4c14dc90191595c97ad3a019d618e7158&end=9ea3878dfec9b20993eaebfdb7566b29e49c52ab&stat=wall-time

But they were eaten up immediately by switching to ThinLTO in the next PR 😁😭

@alexcrichton alexcrichton deleted the compress-parallel branch October 28, 2017 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants