Don't allocate DepNode if anonymous

[arora-aman]

#45408

[rust-highfive]

r? @oli-obk

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

[jonas-schievink]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 1888747508e96b9bb9c50679c6d1dec57ce551c4 with merge 5949e52c59d14b10fb338c73e4e37a41bc36131a...

[bjorn3]

You can use

let dep_node_index = if let Some(dep_node_index) = dep_node_index {
    dep_node_index
} else {
    return ...
};

[arora-aman]

fair, I'll update once the perf run goes through

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 5949e52c59d14b10fb338c73e4e37a41bc36131a (5949e52c59d14b10fb338c73e4e37a41bc36131a)

[rust-timer]

Queued 5949e52c59d14b10fb338c73e4e37a41bc36131a with parent e0bc267, future comparison URL.

[rust-timer]

Finished benchmarking try commit (5949e52c59d14b10fb338c73e4e37a41bc36131a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[bjorn3]

Up to 0.4% slowdown.

[crlf0710]

Triage: Is this ready for review? There's still [WIP] in the PR title.

[bjorn3]

Suggested change

	fn is_anon(&self) -> bool {
	#[inline]
	fn is_anon(&self) -> bool {

can_reconstruct_query_key also seems to be missing it. Or maybe it is missing for a reason?

[arora-aman]

Triage: Is this ready for review? There's still [WIP] in the PR title.

@wesleywiser are we planning on merging this change? If so I can push a fixup.

[wesleywiser]

@arora-aman Don't we still need to do this? #45408 (comment)

I tried building locally and the incremental cache seems to be the same size before and after this change. I would assume we should see some kind of change if the nodes are no longer being allocated.

[oli-obk]

r? @wesleywiser

I have no idea what this PR is about 😆

[tgnottingham]

Regarding the overall approach, we can't remove the anonymous nodes without compensating for their removal (most importantly, copying their dependencies up to the things that depend on them). I've made comments on #45408 discussing some of the issues involved. TL;DR, I think the problem to be solved is a bit harder than it first appears, and solving it isn't necessarily a clear win.

Regarding the code, since I suspect the overall approach will change, my comments are probably totally irrelevant in the end. But here they are nonetheless.

• with_task_impl is supposed to return the allocated node index. If we're not allocating a node, it would be incorrect to say that we were by returning an index. And you wouldn't want to use next_virtual_depnode_index. That's only used when the dep graph is disabled (i.e. non-incremental mode unless some debug options are enabled). If we return that, it'll end up being the index of some other node in the graph most likely.

• That's all irrelevant though, because that code doesn't ever execute (I'm pretty certain anyway). Anonymous queries are executed using with_anon_task, which doesn't use with_task_impl. In fact, it's probably an error if with_task_impl is called with an anonymous node.

  Given that, it seems appropriate to have finish_task_and_alloc_depnode not return an Option, since the code isn't equipped to handle that case. Callers that create the closure can do the unwrapping of the Option returned by alloc_node. You could also just go back to unwrapping in with_task_impl, but it seems silly to suggest to clients that finish_task_and_alloc_depnode can really return None, when we're just going to panic if it does.

• The reason you aren't seeing a reduction in size is that anonymous queries use intern_node, not alloc_node. See complete_anon_task.

  That being said, I think it should be using alloc_node. intern_node is used when we think the graph might already contain the node. I don't think that's the case here. I don't think we should be executing the anonymous query if its node is already in the graph. Somewhere higher in the stack, we should have been using the query's cached result.

[wesleywiser]

Let's go ahead and close this as @arora-aman suggested on Zulip since they're not going to have time to work on this more in the near future.

@arora-aman if want to continue working on this, feel free to re-open at any point!