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

Prefetch some queries used by the metadata encoder #67888

Merged
merged 7 commits into from
Mar 21, 2020

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 5, 2020

This brings the time for metadata encoding and writing for syntex_syntax from 1.338s to 0.997s with 6 threads in non-incremental debug mode.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2020
@rust-highfive

This comment has been minimized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 11, 2020

I did some more tuning and brought the time for metadata encoding and writing down to 0.561.

@michaelwoerister Do you know why the incremental test failed here given that this PR doesn't change dependencies?

@rust-highfive

This comment has been minimized.

@michaelwoerister
Copy link
Member

@michaelwoerister Do you know why the incremental test failed here given that this PR doesn't change dependencies?

I don't. The change doesn't look like should break that test.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 13, 2020

Looks like we don't check queries which did not execute, and this caused some promoted_mir queries to execute.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

I am feeling uncertain about this PR. I would appreciate getting some wider feedback (not sure from who, maybe @michaelwoerister)... it feels like while it does give us some good wins, it feels somewhat fragile (i.e., depends on how metadata encoding works pretty closely).

I would rather see us explore making metadata encoding itself more parallel -- IIRC, the basic idea with encoding is a bunch of arrays representing trait impls, MIR, etc. -- maybe we can instead make constructing those be parallel?

i = self.position();
let exported_symbols = self.tcx.exported_symbols(LOCAL_CRATE);
let exported_symbols = self.encode_exported_symbols(&exported_symbols);
let exported_symbols_bytes = self.position() - i;
Copy link
Member

Choose a reason for hiding this comment

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

Could this commit be fleshed out with some description of why this is done? (i.e., in the commit message ideally)?

Right now it looks like presumably it's to make sure the exported_symbols query can fallback on the parallel MIR optimization in the last commit... but I'm not sure.

tcx.promoted_mir(def_id);
})
},
|| tcx.exported_symbols(LOCAL_CRATE),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, so was this why the previous commit moved exported symbols later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's moved later to give more time for prefetching to happen.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 21, 2020

I would rather see us explore making metadata encoding itself more parallel -- IIRC, the basic idea with encoding is a bunch of arrays representing trait impls, MIR, etc. -- maybe we can instead make constructing those be parallel?

I'd like to remove the existing metadata and all related code and instead use the incremental query cache for both metadata and incremental compilation so I don't really want to put any effort into refactoring the existing code.

@Mark-Simulacrum
Copy link
Member

Should we then not land this either? Feels like the 0.4 second win is nice but not huge, and presumably would be less in incremental mode (more data to load?).

I feel like replacing metadata with incremental query cache is a pretty far reaching goal though -- maybe worth trying to polish metadata into better shape in the mean time? But I can see us not wanting to spend time on it. Obviously out of scope for this PR.

I guess I'm not opposed to landing this PR -- but I would like to see the first review comment addressed (expanding on the commit).

@michaelwoerister
Copy link
Member

Here are some thoughts:

  • The changes seem relatively safe as far as correctness is concerned (although I would add a comment that tcx.dep_graph.with_ignore() is only safe because query results aren't accessed).
  • Generally, doing prefetching in a parallel setting also makes sense.
  • However, the PR does add a bit of complexity and duplicates some logic, and
  • we only have one performance number of one crate (that is known to be a bit of an edge case) in one compilation mode from a single machine without context (e.g. by what percentage did the end-to-end compile time for the crate change). So we don't have a lot of data to base this decision on; and we won't get more even after merging (at least not from perf.rlo).

So I'm on the fence on whether I think this is worth the trouble or not. Since the changes are safe and can be easily reverted, I'd say it's OK to merge but maybe with more comments, i.e.:

  • marking the duplicated logic as such and referring to the respective other occurrences that need to be kept in sync).
  • adding a comment that this prefetching is non-essential and can just be removed if it causes trouble or has detrimental effects.
  • adding a comment about tcx.dep_graph.with_ignore()

@Mark-Simulacrum
Copy link
Member

I am also not opposed to merging with more comments.

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2020
@joelpalmer
Copy link

Triaged

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 14, 2020

I added some comments and make the code use assert_ignored instead of with_ignore.

@Mark-Simulacrum
Copy link
Member

The changes look reasonable, but I cannot review the prefetching of the MIR bodies, as I'm not familiar enough with the code that'll be using that prefetching later on (nor with the relevant queries). I'm a little worried by the amount of code that is needed for prefetching there, too, particularly as it seems likely to not get updated over time (given the complex conditionals especially) to fit exactly what we need.

With that in mind, let's try r? @matthewjasper perhaps? I'm not sure if you're the best person for the optimized/promoted MIR queries, which seem to be dominant in that convoluted code.

@bors
Copy link
Contributor

bors commented Mar 19, 2020

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

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 19, 2020

📌 Commit 027c8d9 has been approved by matthewjasper

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 19, 2020
…sper

Prefetch some queries used by the metadata encoder

This brings the time for `metadata encoding and writing` for `syntex_syntax` from 1.338s to 0.997s with 6 threads in non-incremental debug mode.

r? @Mark-Simulacrum
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#67888 (Prefetch some queries used by the metadata encoder)
 - rust-lang#69934 (Update the mir inline costs)
 - rust-lang#69965 (Refactorings to get rid of rustc_codegen_utils)
 - rust-lang#70054 (Build dist-android with --enable-profiler)
 - rust-lang#70089 (rustc_infer: remove InferCtxt::closure_sig as the FnSig is always shallowly known.)
 - rust-lang#70092 (hir: replace "items" terminology with "nodes" where appropriate.)
 - rust-lang#70138 (do not 'return' in 'throw_' macros)
 - rust-lang#70151 (Update stdarch submodule)

Failed merges:

 - rust-lang#70074 (Expand: nix all fatal errors)

r? @ghost
@bors bors merged commit 9adfb18 into rust-lang:master Mar 21, 2020
@Zoxc Zoxc deleted the metadata-prefetch branch March 21, 2020 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants