Refactor ensure and try_get_with

[theotherjimmy]

There was a bit of code shared between try_get_with and ensure, after I
added ensure. I refactored that shared code into a query-agnostic method
called read_node_index.

The new method read_node_index will attempt to find the node
index (DepNodeIndex) of a query. When read_node_index finds the
DepNodeIndex, it marks the current query as a reader of the node it's
requesting the index of.

This is used by try_get_with and ensure as it elides the unimportant (to
them) details of if the query is invalidated by previous changed computation (Red)
or new and if they had to mark the query green. For both try_get_with and
ensure, they just need to know if they can lookup the results or have to
reevaluate.

@nikomatsakis this is the refactor we discussed in the comment thread of #45228

[rust-highfive]

r? @arielb1

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

[michaelwoerister]

Thanks for the PR, @theotherjimmy!

I like the code deduplication. However could you:

• move the function to plumbing.rs and make it private there? I'd like to publicly expose as little functionality as possible from DepGraph. I don't want to give the wrong impression that anybody is supposed to directly call DepGraph methods (unless they really know what they are doing).
• rename the function to better reflect what it is actually doing? It can be something verbose like try_mark_green_and_read().

[theotherjimmy]

move the function to plumbing.rs and make it private there?

The argument for privacy makes sense and I'll move it. The reason that I put it where I did, is that it did not need to be duplicated for each query type.

rename the function to better reflect what it is actually doing?

Ah, the hardest problem in computer science: naming. try_mark_green_and_read() is fine for a private function, but I would like public ones to be a tad less verbose.

Another commit is on the way with the move and rename. Don't hold your breath, it may be a few hours.

[michaelwoerister]

The reason that I put it where I did, is that it did not need to be duplicated for each query type.

You could try and see if you can make this a free-standing function in plumbing.rs, outside of the macro. Then it would not be duplicated. You'll probably have to make it pub(super) then.

[theotherjimmy]

Thanks for the tip. I'll give that a try.

[theotherjimmy]

@michaelwoerister I pushed up a commit that should address your review comments. I also moved the implementation of try_mark_green_and_read into TyCtxt as it took in a TyCtxt and a DepGraph, which is redundant as TyCtxt contains a DepGraph. This also has the benefit of reducing the number of arguments to the try_mark_green_and_read method.

[theotherjimmy]

Please excuse the rebase. I forgot to delete the old implementation of read_node_index from graph.rs.

[michaelwoerister]

Thanks @theotherjimmy! I think you've found the right place to put the function.

@bors r+

[bors]

📌 Commit 229bee3 has been approved by michaelwoerister

[bors]

⌛ Testing commit 229bee3 with merge 07a3441...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry

• dist-i586-gnu-i686-musl spurious test segfault Spurious segfault in tests for musl #38618

[bors]

⌛ Testing commit 229bee3 with merge 354eb16...

[bors]

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