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.

Sign up for GitHub

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

Jump to bottom

MetadataOnlyCodegenBackend: run the collector only once #57418

Merged
merged 2 commits into from
Jan 14, 2019
Merged

MetadataOnlyCodegenBackend: run the collector only once #57418

merged 2 commits into from
Jan 14, 2019

Conversation

lqd
Copy link
Member

@lqd lqd commented Jan 7, 2019 •
edited
Loading

Use the collect_and_partition_mono_items query to avoid calling the collector directly twice.

Fixes #57406.

@lqd
Codegen: run the collector only once
c075e24 
Use the `collect_and_partition_mono_items ` query to avoid calling the collector
@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 Jan 7, 2019
nagisa
nagisa reviewed Jan 7, 2019
View reviewed changes
src/librustc_codegen_utils/codegen_backend.rs
@@ -136,24 +135,15 @@ impl CodegenBackend for MetadataOnlyCodegenBackend {
::symbol_names_test::report_symbol_names(tcx);
::rustc_incremental::assert_dep_graph(tcx);
::rustc_incremental::assert_module_sources::assert_module_sources(tcx);
::rustc_mir::monomorphize::assert_symbols_are_distinct(tcx,
Copy link
Member

Choose a reason for hiding this comment

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

Note to anybody looking through this: collect_and_partition_mono_items query does this assert internally.

@cramertj
Copy link
Member

cramertj commented Jan 7, 2019

r? @michaelwoerister

@lqd
Copy link
Member Author

lqd commented Jan 7, 2019

(At least this basic first Travis pass seems to work, so I'm removing the WIP-ness)

@lqd lqd changed the title [WIP] codegen-backend: run the collector only once codegen-backend: run the collector only once Jan 7, 2019
@michaelwoerister
Copy link
Member

Thanks a lot, @lqd! Let's see if we can notice a performance difference.

@bors try

@bors
Copy link
Contributor

bors commented Jan 8, 2019

⌛ Trying commit c075e24 with merge 85cebe1...

bors added a commit that referenced this pull request Jan 8, 2019
@bors
Auto merge of #57418 - lqd:collector_query, r=<try>
85cebe1 
codegen-backend: run the collector only once

Use the `collect_and_partition_mono_items` query to avoid calling the collector directly twice.

Fixes #57406.
@bors
Copy link
Contributor

bors commented Jan 8, 2019

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member

@rust-timer build 85cebe1

@rust-timer
Copy link
Collaborator

Success: Queued 85cebe1 with parent 9d54812, comparison URL.

@bjorn3
Copy link
Member

bjorn3 commented Jan 8, 2019 •
edited
Loading

This will not have any perf change, because this is not the llvm codegen backend, but a backend only emitting metadata introduced in #44085. You have to pass -Zcodegen-backend=metadata_only to use it. Its probably broken at the moment anyway.

Edit: correct debug flag.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 85cebe1

@michaelwoerister
Copy link
Member

Oh, it's a different backend...

Well, does it rely on collection being eager? Why does it even monomorphize items in the first place?

@bjorn3
Copy link
Member

bjorn3 commented Jan 8, 2019

Well, does it rely on collection being eager?

Probably not.

Why does it even monomorphize items in the first place?

I tried to make as many tests pass using it.

@michaelwoerister
Copy link
Member

Since the results of the method calls in the loop are not used, I assume the calls are only there for triggering diagnostics.

  • The inst.def.is_inline(tcx) call does not seem to be needed, since it has no side effects.
  • The codegen_fn_attrs query does some validation. It would, however, work just as well without doing monomorphization. Depending on whether this backend is used in a performance-critical scenario, it would make sense to try and acquire the list of relevant DefIds without running the monomorphization collector. tcx.hir().krate().visit_all_item_likes() should do the trick.

@lqd lqd changed the title codegen-backend: run the collector only once MetadataOnlyCodegenBackend: run the collector only once Jan 9, 2019
@bjorn3
Copy link
Member

bjorn3 commented Jan 9, 2019

Since the results of the method calls in the loop are not used, I assume the calls are only there for triggering diagnostics.

Yes.

Depending on whether this backend is used in a performance-critical scenario

As far as I know it hasn't really been used by anyone (including me). It can be removed if you want.

@lqd
MetadataOnlyCodegenBackend: remove is_inline call
e5318f3 
This function has no side effects, and its result is ignored.
@michaelwoerister
Copy link
Member

OK, let's merge the current version for now. This is an unused component, I don't want to put too much time into it.

Thanks for looking into it, @lqd!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 14, 2019

📌 Commit e5318f3 has been approved by michaelwoerister

@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 Jan 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019
@Centril
Rollup merge of rust-lang#57418 - lqd:collector_query, r=michaelwoeri…
3e869d7 
…ster

MetadataOnlyCodegenBackend: run the collector only once

Use the `collect_and_partition_mono_items` query to avoid calling the collector directly twice.

Fixes rust-lang#57406.
Centril added a commit to Centril/rust that referenced this pull request Jan 14, 2019
@Centril
Rollup merge of rust-lang#57418 - lqd:collector_query, r=michaelwoeri…
36d68f5 
…ster

MetadataOnlyCodegenBackend: run the collector only once

Use the `collect_and_partition_mono_items` query to avoid calling the collector directly twice.

Fixes rust-lang#57406.
@Centril Centril mentioned this pull request Jan 14, 2019
Merged
bors added a commit that referenced this pull request Jan 14, 2019
@bors
Auto merge of #57592 - Centril:rollup, r=Centril
d106808 
Rollup of 6 pull requests

Successful merges:

 - #57232 (Parallelize and optimize parts of HIR map creation)
 - #57418 (MetadataOnlyCodegenBackend: run the collector only once)
 - #57465 (Stabilize cfg_target_vendor)
 - #57477 (clarify resolve typo suggestion)
 - #57556 (privacy: Fix private-in-public check for existential types)
 - #57584 (Remove the `connect_timeout_unroutable` test.)

Failed merges:

r? @ghost
@bors bors merged commit e5318f3 into rust-lang:master Jan 14, 2019
@lqd lqd deleted the collector_query branch January 14, 2019 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lqd @rust-highfive @cramertj @michaelwoerister @bors @rust-timer @bjorn3 @nagisa