Refactor the query system to reduce the amount of macro-expanded functions

[cjgillot]

The current implementation of the query system relies on a lot of macro-expanded functions.
This issue tracks perspectives to reduce this use of macros.

The objective of this issue is to have the macros expand to a self-contained description of the query, instead of breadcrumbs in several files.

• Remove opt_remap_env_constness from rustc_query_impl (Remove opt_remap_env_constness from rustc_query_impl #100243)

The interface code in rustc_middle::ty::query can be made responsible for calling it, it already does.

• Simplify QueryDescription trait

TRY_LOAD_FROM_DISK, cache_on_disk and describe are only used for
the QueryVtable and make_query::$name. They can be made inherent
associated constant and functions on the queries::$name types.

• Move Value trait to rustc_query_system (Make HandleCycleError an enum instead of a macro-generated closure #101303)

This would allow to replace the function pointer for handle_cycle_error by a simple enum.

• Refactor make_query::$name functions to a generic version instead of a macro (Move most of TyCtxtAt::$name into a generic evaluate_query function #101178)

The call to opt_remap_env_constness! is unnecessary. The DepKind can be passed as a parameter. describe can be passed as a function pointer. key can be made an impl Key + HashStable.

We should consider moving part of the code into QueryState::try_collect_active_jobs which is the only user of these functions.

• Move DepKindStruct to rustc_query_system (Move DepKindStruct from rustc_middle to rustc_query_system #101710)

DepKindStruct is defined in rustc_middle::dep_graph::dep_node. It depends on TyCtxt and DepKind, but that can be replaced by a generic parameter CTX: DepContext and CTX::DepKind.

rustc_query_system would access it through a new DepContext::dep_kind_info providing fingerprint_style, is_eval_always, try_force_from_dep_node and try_load_from_on_disk_cacke, and replacing TyCtxt::query_kind.

If specialization allows it, we should aim to make most of the code in rustc_middle::dep_graph::dep_node generic over CTX: DepContext, and move it to rustc_query_system.

• Introduce an array of QueryStruct with function pointers (Use function pointers instead of macro-unrolled loops in rustc_query_impl #101785)

Having macro-unrolled loops prevents extension of the infrastructure to non-statically known instances. This struct is to be used in a similar way to DepKindStruct, but with types private to rustc_query_impl.

struct QueryStruct {
    try_collect_active_jobs: fn(QueryCtxt<'_>, &mut QueryMap) -> Option<()>,
    alloc_self_profile_query_strings: fn(QueryCtxt<'tcx>, &mut QueryKeyStringCache),
    encode_query_results: Option<fn(QueryCtxt<'_>, &mut CacheEncoder<...>, &mut EncodedDepNodeIndex)>
}

This will be used to:

• replace the macro expansion in try_collect_active_jobs by a loop over function pointers;
• replace the macro expansion in alloc_self_profile_query_strings by a loop over function pointers;
• replace the macro expansion in encode_query_results by a loop over function pointers.

The array itself should be created using make_dep_kind_array! which handles the indices.
Possible variant: put the try_collect_active_jobs and alloc_self_profile_query_strings function pointers in DepKindStruct instead.

Please contact me on Zulip for further information.

[kckeiks]

@rustbot claim

[kckeiks]

Update: I started working on this a while back but I got pulled away by other things. I will have more bandwidth in the next couple of weeks and hopefully have a PR.

[jyn514]

@kckeiks what parts of this issue are you currently working on? don't want to step on your toes, but I think there's enough work here we can split it up between us and have plenty of work for both :)

[kckeiks]

@jyn514 I had started working on "Simplify QueryDescription trait" and looking into "Refactor make_query::$name functions to a generic version instead of a macro". I was planning on continuing working on these tasks in the next 1-2 weeks when I would have more bandwidth. Please let me know what you plan to pick up.

[jyn514]

Cool :) I've already done the make_query bits I think: #100943
I'll let you know if I start working on anything else, I'll avoid QueryDescription for now.

[jyn514]

I had an idea for how to change the 3 nested macros (rustc_queries -> rustc_query_append -> define_callbacks) but after spending a long time writing it up, I no longer think it's a good idea. Posting it below anyway because maybe it will give others ideas lol

I've realized one of the reasons rustc_queries! is so complicated is because it's a proc macro with multiple outputs; which isn't natively supported by the language, so we hack it by defining multiple macros, one for each output. I wonder if it would be simpler to define a single trait + trait impls instead, which the rest of rustc_query_impl::plumbing can use rather than the macro outputs.

The tradeoff is that we'll have fewer layers of macros, at the cost of moving more code into the proc-macro, which makes it harder to refactor. define_callbacks is the main one that would add complexity to rustc_queries; see below.

Here are the uses of the macros I see today:

• rustc_query_description! { $name } in impl<'tcx> QueryDescription<QueryCtxt<'tcx>> for queries::$name<'tcx> { (inside define_queries). If the QueryDescription impl is generated by the proc-macro this will no longer be necessary at all.
• rustc_cached_queries!(encode_queries!) in encode_query_results. I think we can avoid this by having an static ALL_QUERIES: &[&dyn QueryDescription] = [ ... ]; array generated by the macro; we'd loop over the array rather than having the macro generate static calls. That may have a runtime impact due to the new function pointer, but we might make it up in icache. Not sure how hot encode_query_results is.
• rustc_dep_node_append!(define_dep_nodes![]) in compiler/rustc_middle/src/dep_graph/dep_node.rs. This one seems harder - label_strs is easy to avoid by adding a const NAME: &'static str; item to QueryDescription, but adding custom Null/Red/TraitSelect variants to the enum DepKind seems harder. Maybe those can be defined by the proc-macro, rather than added after the fact?
• rustc_query_append! { define_queries! } in compiler/rustc_query_impl/src/lib.rs. IMO this already does 99% of what I'd want the proc-macro to do; not many changes needed here
• rustc_query_append! { alloc_once! } in alloc_self_profile_query_strings. this is not perf sensitive and only uses the macro name; the ALL_QUERIES list would work here too.
• rustc_query_append! { define_callbacks! } in compiler/rustc_middle/src/ty/query.rs. This would have to be directly defined by the proc-macro, I think; not sure how much is simplified there :/ This is quite a large macro too (200+ lines) so it's adding a great deal of complexity to the proc-macro.

[jyn514]

The tradeoff is that we'll have fewer layers of macros, at the cost of moving more code into the proc-macro, which makes it harder to refactor. define_callbacks is the main one that would add complexity to rustc_queries; see below.

oh heh, this is what @eddyb suggested 4 years ago #56462 (comment)

[jyn514]

Possible variant: put the try_collect_active_jobs and alloc_self_profile_query_strings function pointers in DepKindStruct instead.

I think this won't be possible after #101710, because DepKindStruct will be defined in rustc_query_system which doesn't have access to QueryCtxt. I guess it could use type parameters though.

[Noratrieb]

Releasing assignment due to inactivity, feel free to claim it again. @rustbot release-assignment

[kckeiks]

Yeah sorry, I got busy with work :(. If no one picks it up in the summer, I should have some time to try to tackle it then.