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

Don't copy bytecode files into the incr. comp. cache. #71754

Merged
merged 1 commit into from
May 4, 2020

Conversation

alexcrichton
Copy link
Member

It's no longer necessary now that bitcode is embedded into object files.

This change meant that WorkProductFileKind::Bytecode is no longer
necessary, which means that type is no longer necessary, which allowed
several places in the code to become simpler.

This commit was written by @nnethercote in #70458 but that didn't land. In the meantime though we managed to land it in #71528 and that doesn't seem to be causing too many fires, so I'm re-sending this patch!

It's no longer necessary now that bitcode is embedded into object files.

This change meant that `WorkProductFileKind::Bytecode` is no longer
necessary, which means that type is no longer necessary, which allowed
several places in the code to become simpler.
@rust-highfive
Copy link
Collaborator

r? @cramertj

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2020
@alexcrichton
Copy link
Member Author

r? @nnethercote

@rust-highfive rust-highfive assigned nnethercote and unassigned cramertj May 1, 2020
@nnethercote
Copy link
Contributor

Looks fine, but I'd like to see a perf run first, just to know if it has much effect.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 1, 2020

⌛ Trying commit d4e5e1b with merge 032fb238cef1c57960742f33db0829b5f52a23a1...

@bors
Copy link
Contributor

bors commented May 2, 2020

☀️ Try build successful - checks-azure
Build commit: 032fb238cef1c57960742f33db0829b5f52a23a1 (032fb238cef1c57960742f33db0829b5f52a23a1)

@rust-timer
Copy link
Collaborator

Queued 032fb238cef1c57960742f33db0829b5f52a23a1 with parent 7f65393, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 032fb238cef1c57960742f33db0829b5f52a23a1, comparison URL.

@Mark-Simulacrum
Copy link
Member

Performance looks essentially neutral, at least by instruction counts - which is what we would expect, I think, as copying into the incr comp cache isn't heavy on instructions presumably.

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented May 2, 2020

📌 Commit d4e5e1b has been approved by nnethercote

@bors bors 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 May 2, 2020
@nnethercote
Copy link
Contributor

The wall time results are noisy as always, but look like a slight improvement.

@bors
Copy link
Contributor

bors commented May 4, 2020

⌛ Testing commit d4e5e1b with merge 649b632...

@bors
Copy link
Contributor

bors commented May 4, 2020

☀️ Test successful - checks-azure
Approved by: nnethercote
Pushing 649b632 to master...

pub enum WorkProductFileKind {
Object,
Bytecode,
pub saved_files: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

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

This could become Option<String> as only a single object file will ever be saved for a codegen unit.

@alexcrichton alexcrichton deleted the no-bitcode-in-cache branch July 23, 2020 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants