test caching opt_const_param_of on disc

[lcnr]

Followup to #74113, implements parts of #74360

Tried caching opt_const_param_of on disk and adding an early exit if tcx.dep_kind(def_id) != DefKind::AnonConst.

Ended up causing a perf regression instead, so we just remove the FIXME and a short note to opt_const_param_of.

r? @eddyb

[lcnr]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 620d6883fd74efedbb67ac019186c6e76c826ea5 with merge 2e498501f591a1be258a953ea1ed225408909730...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 2e498501f591a1be258a953ea1ed225408909730 (2e498501f591a1be258a953ea1ed225408909730)

[rust-timer]

Queued 2e498501f591a1be258a953ea1ed225408909730 with parent 7e11379, future comparison URL.

[eddyb]

I think this is fine, curious if perf will change.

[rust-timer]

Finished benchmarking try commit (2e498501f591a1be258a953ea1ed225408909730): comparison url.

[lcnr]

Looks like perf is worse 🤔 I think DefKind might actually increase the incremental deps, as opt_const_param_of currently only looks into Node::AnonConst. Let's try it without using DefKind.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 016205678946e3483aac1bd53417ac3c8d867296 with merge 374482ebbce140f884a62148e2a93c7ceee04adf...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 374482ebbce140f884a62148e2a93c7ceee04adf (374482ebbce140f884a62148e2a93c7ceee04adf)

[rust-timer]

Queued 374482ebbce140f884a62148e2a93c7ceee04adf with parent e2e29de, future comparison URL.

[eddyb]

Heh, at least all the regressions are in non-initial incremental runs.

I think DefKind might actually increase the incremental deps, as opt_const_param_of currently only looks into Node::AnonConst

But when you request a HIR node, you depend on everything in it (I guess not on the body of things that have bodies, so maybe it's fine - it's similar to type_of or fn_sig in that way).

[lcnr]

def_kind also always requests a Hir node. def_kind also does stuff with other Nodes which should increase the complexity of the dependency tree of opt_const_param_of.

[eddyb]

@lcnr the cost comes from tcx.hir().get(...), it doesn't matter what you access inside, the fact that you got the Node means whatever you're in will get reexecuted if anything in that HIR owner (but outside bodies I guess) changes.

def_kind acts like a "firewall", in that if opt_const_param_of uses it for the early return, opt_const_param_of will only be recomputed if def_kind's value changes.

E.g. opt_const_param_of(some_function) would normally get recomputed when the signature changes, but with the def_kind early-exit in place, it won't be recomputed when the signature changes because the value of def_kind(some_function) hadn't changed (it returns DefKind::Fn both times, and the second time opt_const_param_of isn't even executed).

But apparently it's not worth it, i.e. recomputing opt_const_param_of (when it returns None) is cheap enough, and I guess that makes sense.

Although, I wonder if the perf results are from caching on disk, that seems like it would matter more than the early return.

[lcnr]

My reasoning is that def_kind calls get_parent_node of Node::Ctor. So if the parent_node changes for def_kind get's invalidated while the current opt_const_param_of does not.

I kind of doubt that that's relevant here though, the perf impact probably just comes from caching it on disc at all 😅

[eddyb]

def_kind being invalidated is fine, because that won't invalidate opt_const_param_of unless the value changes.
That is, dependencies are not transitive, every step of the way can block the propagation of changes.
This is also known as "red-green" incremental.

[rust-timer]

Finished benchmarking try commit (374482ebbce140f884a62148e2a93c7ceee04adf): comparison url.

[lcnr]

🤷 let's just add a note to opt_const_param_of and call it a day here 😆

[lcnr]

ready for merge I guess

[eddyb]

r=me with title/description updated to match the current state

[eddyb]

@bors r+ rollup

[bors]

📌 Commit e8d16fd has been approved by eddyb