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

Turn HIR indexing into a query #59064

Closed
wants to merge 3 commits into from
Closed

Turn HIR indexing into a query #59064

wants to merge 3 commits into from

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 10, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2019
@@ -458,6 +458,7 @@ define_dep_nodes!( <'tcx>
[eval_always] PrivacyAccessLevels(CrateNum),
[eval_always] CheckPrivateInPublic(CrateNum),
[eval_always] Analysis(CrateNum),
[eval_always] HirMap(CrateNum),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably need a true eval_always query type, since these are actually eval if HIR changes, and we don't know if the HIR changes until after the HIR is hashed, which happens in the HirMap query.

Copy link
Member

Choose a reason for hiding this comment

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

This refers to #59091, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@Zoxc Zoxc changed the title [WIP] Turn HIR indexing into a query Turn HIR indexing into a query Mar 15, 2019
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 15, 2019

@bors try

@bors
Copy link
Contributor

bors commented Mar 15, 2019

⌛ Trying commit 30e23f6 with merge eb9092d...

bors added a commit that referenced this pull request Mar 15, 2019
@bors
Copy link
Contributor

bors commented Mar 15, 2019

☀️ Try build successful - checks-travis
Build commit: eb9092d

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 15, 2019

@rust-timer build eb9092d

@rust-timer
Copy link
Collaborator

Success: Queued eb9092d with parent 70d1150, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit eb9092d

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 16, 2019

Looks like caching the HIR query is effectively removes the performance penalty from making it a query. These results are pretty much identical to #59091. (compare with no caching in #57024).

@bors
Copy link
Contributor

bors commented Mar 19, 2019

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

@Zoxc Zoxc force-pushed the query-hir branch 2 times, most recently from ddfc6c3 to 25b731e Compare March 19, 2019 09:39
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

You are suggesting that our eval_always queries are not true eval_always queries, can you explain/document this in more detail?

@@ -601,9 +601,13 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub(super) fn ensure_query<Q: QueryDescription<'gcx>>(self, key: Q::Key) -> () {
let dep_node = Q::to_dep_node(self, &key);

// Ensuring an "input" or anonymous query makes no sense
if dep_node.kind.is_eval_always() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does ensuring an eval_always query make sense? No data should be cached by doing this, so at best we see some diagnostics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensuring queries just make sure the side effects of the query has been applied. This also applies to eval_always queries. However it doesn't make sense trying to mark them green with try_mark_green_and_read because we don't record dependencies for them, so they are handled separately here.

src/librustc/ty/context.rs Show resolved Hide resolved
src/librustc/ty/context.rs Outdated Show resolved Hide resolved

pub hir_defs: hir::map::Definitions,

hir_map: AtomicCell<Option<&'tcx hir_map::Map<'tcx>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use Once here?

@@ -1082,7 +1089,17 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {

#[inline(always)]
pub fn hir(self) -> &'a hir_map::Map<'gcx> {
&self.hir_map
let value = self.hir_map.load();
if unlikely!(value.is_none()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be simplified significantly by the use of Once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once is slow, and the hir function is very hot.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, can you at least fold this into your AtomicCell (by passing a closure to some method) so the non-query code stays out of the query?

Since the AtomicCell is just used for this, maybe rename it to AtomicOnce and hide the Option from the use site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AtomicCell is a crossbeam-utils type. I can't modify it.

non-query code stays out of the query

Not sure what you mean here. This is just a regular function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw your version in rustc_datastructures, but I realize now that's just for nonparallel rustc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean here. This is just a regular function.

well it's like an emulated query. It would be a regular query just for the caching if it weren't so hot or had any arguments, right?

Which reminds me. Doesn't this being a memoization mean we'll not have an edge from queries calling hir to the hir_map query, except for the first query to call hir? Is that problematic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

except for the first query to call hir?

It uses with_ignore so there won't be any edges even for the first one. Having edges to the real hir_map query would be bad, as it is no_hash and eval_always which makes it effectively always red (and everything which depends on it always execute).

Not adding edges here is fine since the HIR map adds edges when you use it to access things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ending up using this pattern in a couple more places so I'm adding a AtomicOnce type for it.

@@ -388,10 +388,7 @@ impl DepGraph {
|_| None,
|data, key, fingerprint, _| {
let mut current = data.borrow_mut();
let krate_idx = current.node_to_node_index[
&DepNode::new_no_params(DepKind::Krate)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eval_always queries are hardcoded to depend on the entire HIR (Krate) here. I changed these to instead always execute instead of executing only when the HIR changes. Also see comments / perf on #59091

@bors
Copy link
Contributor

bors commented Mar 26, 2019

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

@Zoxc Zoxc force-pushed the query-hir branch 2 times, most recently from 2db0fbe to df25514 Compare March 27, 2019 16:20
@bors
Copy link
Contributor

bors commented May 1, 2019

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

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 9, 2019

We discussed this in the triage meeting. There were a few points raised:

  • This PR itself is likely fairly simple and not worthy of meeting all by its lonesome
  • But many of us would like to see the overall direction sketched out
  • We'll need to find the right people to do the reviewing (@oli-obk seemed interested)
  • Niko (me!) feels like this ought to be a working group we can check in on periodically

So I'm removing nomination but in general we would like to hold up until we've had agreement on the overall "end-to-end" plan.

@bors
Copy link
Contributor

bors commented May 10, 2019

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

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2019
@Mark-Simulacrum
Copy link
Member

Marking as waiting on the compiler team -- the overall plan needs to be sketched out and possibly a meeting scheduled to approve it for implementation. Once that happens, @Zoxc has a series of PRs which will need to be re-opened (#59404, #59338, #59282, and #59205) that depend on this one and each other.

@mark-i-m
Copy link
Member

Has this been discussed at any point?

@pnkfelix
Copy link
Member

triage meeting: we may want to dedicate a planning meeting to resolving the plan here

@bors
Copy link
Contributor

bors commented Jun 14, 2019

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

@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Need to get 2,649 kB of archives.
After this operation, 7,904 kB of additional disk space will be used.
Ign:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
Ign:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
Err:3 http://security.ubuntu.com/ubuntu xenial-security/main amd64 gdb amd64 7.11.1-0ubuntu1~16.5
  Unable to connect to apt.cache.travis-ci.com:http:
Ign:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
Ign:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
Ign:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
Ign:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
Ign:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
Ign:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
Ign:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
Ign:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
Err:1 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace1 amd64 1.3.2-1
  Could not connect to apt.cache.travis-ci.com:80 (34.96.81.152), connection timed out
Err:2 http://archive.ubuntu.com/ubuntu xenial/main amd64 libbabeltrace-ctf1 amd64 1.3.2-1
  Unable to connect to apt.cache.travis-ci.com:http:
Fetched 2,526 kB in 30s (83.2 kB/s)
Fetched 2,526 kB in 30s (83.2 kB/s)
E: Failed to fetch http://archive.ubuntu.com/ubuntu/pool/main/b/babeltrace/libbabeltrace1_1.3.2-1_amd64.deb  Could not connect to apt.cache.travis-ci.com:80 (34.96.81.152), connection timed out
E: Failed to fetch http://archive.ubuntu.com/ubuntu/pool/main/b/babeltrace/libbabeltrace-ctf1_1.3.2-1_amd64.deb  Unable to connect to apt.cache.travis-ci.com:http:
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
travis_fold:start:apt-get.diagnostics
apt-get install failed
apt-get install failed
$ cat ${TRAVIS_HOME}/apt-get-update.log
Get:2 http://security.ubuntu.com/ubuntu xenial-security InRelease [109 kB]
Get:3 http://archive.ubuntu.com/ubuntu xenial-updates InRelease [109 kB]
Get:4 http://archive.ubuntu.com/ubuntu xenial-backports InRelease [107 kB]
Get:4 http://archive.ubuntu.com/ubuntu xenial-backports InRelease [107 kB]
Get:5 http://apt.postgresql.org/pub/repos/apt xenial-pgdg InRelease [51.5 kB]
Get:7 http://security.ubuntu.com/ubuntu xenial-security/restricted Sources [2,243 B]
Get:8 http://security.ubuntu.com/ubuntu xenial-security/universe Sources [131 kB]
Get:9 http://security.ubuntu.com/ubuntu xenial-security/multiverse Sources [3,517 B]
Get:10 http://security.ubuntu.com/ubuntu xenial-security/main amd64 Packages [882 kB]
Get:10 http://security.ubuntu.com/ubuntu xenial-security/main amd64 Packages [882 kB]
Get:11 http://security.ubuntu.com/ubuntu xenial-security/main i386 Packages [722 kB]
Get:12 http://security.ubuntu.com/ubuntu xenial-security/main Translation-en [391 kB]
Get:13 http://security.ubuntu.com/ubuntu xenial-security/restricted amd64 Packages [12.7 kB]
Get:14 http://security.ubuntu.com/ubuntu xenial-security/restricted i386 Packages [12.7 kB]
Get:15 http://security.ubuntu.com/ubuntu xenial-security/restricted Translation-en [2,204 B]
Get:17 http://archive.ubuntu.com/ubuntu xenial/main Sources [1,103 kB]
Get:18 http://security.ubuntu.com/ubuntu xenial-security/universe i386 Packages [491 kB]
Get:19 http://archive.ubuntu.com/ubuntu xenial/restricted Sources [5,179 B]
Get:20 http://security.ubuntu.com/ubuntu xenial-security/universe Translation-en [243 kB]
Get:20 http://security.ubuntu.com/ubuntu xenial-security/universe Translation-en [243 kB]
Get:21 http://archive.ubuntu.com/ubuntu xenial/universe Sources [9,802 kB]
Get:22 http://security.ubuntu.com/ubuntu xenial-security/multiverse amd64 Packages [6,121 B]
Get:23 http://security.ubuntu.com/ubuntu xenial-security/multiverse i386 Packages [6,297 B]
Get:24 http://security.ubuntu.com/ubuntu xenial-security/multiverse Translation-en [2,699 B]
Get:26 http://archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,558 kB]
Get:26 http://archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,558 kB]
Get:27 http://apt.postgresql.org/pub/repos/apt xenial-pgdg/main amd64 Packages [206 kB]
Get:28 http://archive.ubuntu.com/ubuntu xenial/main i386 Packages [1,552 kB]
Get:29 http://apt.postgresql.org/pub/repos/apt xenial-pgdg/main i386 Packages [206 kB]
Get:30 http://archive.ubuntu.com/ubuntu xenial/main Translation-en [799 kB]
Get:32 http://archive.ubuntu.com/ubuntu xenial/restricted i386 Packages [14.5 kB]
Get:32 http://archive.ubuntu.com/ubuntu xenial/restricted i386 Packages [14.5 kB]
Get:33 http://archive.ubuntu.com/ubuntu xenial/restricted Translation-en [3,019 B]
Get:34 http://archive.ubuntu.com/ubuntu xenial/universe amd64 Packages [9,827 kB]
Get:35 http://archive.ubuntu.com/ubuntu xenial/universe i386 Packages [9,804 kB]
Get:36 http://archive.ubuntu.com/ubuntu xenial/universe Translation-en [6,256 kB]
Get:38 http://archive.ubuntu.com/ubuntu xenial/multiverse i386 Packages [172 kB]
Get:39 http://archive.ubuntu.com/ubuntu xenial/multiverse Translation-en [131 kB]
Get:40 http://archive.ubuntu.com/ubuntu xenial-updates/main Sources [427 kB]
Get:41 http://archive.ubuntu.com/ubuntu xenial-updates/restricted Sources [2,696 B]
Get:41 http://archive.ubuntu.com/ubuntu xenial-updates/restricted Sources [2,696 B]
Get:42 http://archive.ubuntu.com/ubuntu xenial-updates/universe Sources [324 kB]
Get:43 http://archive.ubuntu.com/ubuntu xenial-updates/multiverse Sources [9,428 B]
Get:44 http://archive.ubuntu.com/ubuntu xenial-updates/main amd64 Packages [1,265 kB]
Get:45 http://archive.ubuntu.com/ubuntu xenial-updates/main i386 Packages [1,078 kB]
Get:46 http://archive.ubuntu.com/ubuntu xenial-updates/main Translation-en [551 kB]
Get:47 http://archive.ubuntu.com/ubuntu xenial-updates/restricted amd64 Packages [13.1 kB]
Get:48 http://archive.ubuntu.com/ubuntu xenial-updates/restricted i386 Packages [13.1 kB]
Get:49 http://archive.ubuntu.com/ubuntu xenial-updates/restricted Translation-en [2,337 B]
Get:51 http://archive.ubuntu.com/ubuntu xenial-updates/universe i386 Packages [888 kB]
Get:52 http://archive.ubuntu.com/ubuntu xenial-updates/universe Translation-en [430 kB]
Get:53 http://archive.ubuntu.com/ubuntu xenial-updates/multiverse amd64 Packages [19.1 kB]
Get:54 http://archive.ubuntu.com/ubuntu xenial-updates/multiverse i386 Packages [17.9 kB]
Get:54 http://archive.ubuntu.com/ubuntu xenial-updates/multiverse i386 Packages [17.9 kB]
Get:55 http://archive.ubuntu.com/ubuntu xenial-updates/multiverse Translation-en [8,978 B]
Get:56 http://archive.ubuntu.com/ubuntu xenial-backports/main Sources [5,073 B]
Get:57 http://archive.ubuntu.com/ubuntu xenial-backports/universe Sources [7,237 B]
Get:58 http://archive.ubuntu.com/ubuntu xenial-backports/main amd64 Packages [7,942 B]
Get:59 http://archive.ubuntu.com/ubuntu xenial-backports/main i386 Packages [7,942 B]
Get:60 http://archive.ubuntu.com/ubuntu xenial-backports/main Translation-en [4,571 B]
Get:61 http://archive.ubuntu.com/ubuntu xenial-backports/universe amd64 Packages [8,532 B]
Get:62 http://archive.ubuntu.com/ubuntu xenial-backports/universe i386 Packages [8,172 B]
Get:63 http://archive.ubuntu.com/ubuntu xenial-backports/universe Translation-en [4,275 B]
Reading package lists...
travis_fold:end:apt-get.diagnostics
travis_fold:end:apt-get.diagnostics
The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install gdb" failed and exited with 100 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@JohnCSimon
Copy link
Member

Ping from triage
@Zoxc Can you please address the merge conflicts and change requests?
Thank you.

@Dylan-DPC-zz
Copy link

@Zoxc any updates on this? Is there anything this is still blocked on? Thanks

@Zoxc
Copy link
Contributor Author

Zoxc commented Nov 25, 2019

It's blocked on compiler team consensus. There is a meeting scheduled to talk about this.

@Zoxc Zoxc closed this Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.