Allow arena_cache queries to return Option<&'tcx T>
#135911
Conversation
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Zalathar
force-pushed
the
arena-cache-option
branch
from
6558734 to
4c448d5
Compare
|
I don't expect to see a measurable reduction in memory use, but I'm curious. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Allow `arena_cache` queries to return `Option<&'tcx T>` Currently, `arena_cache` queries always have to return `&'tcx T`[^deref]. This means that if an arena-cached query wants to return an optional value, it has to return `&'tcx Option<T>`, which has a few negative consequences: - It goes against normal Rust style, where `Option<&T>` is preferred over `&Option<T>`. - Callers that actually want an `Option<&T>` have to manually call `.as_ref()` on the query result. - When the query result is `None`, a full-sized `Option<T>` still needs to be stored in the arena. This PR solves that problem by introducing a helper trait `ArenaCached` that is implemented for both `&T` and `Option<&T>`, and takes care of bridging between the provided type, the arena-allocated type, and the declared query return type. --- To demonstrate that this works, I have converted the two existing arena-cached queries that currently return `&Option<T>`: `mir_coroutine_witnesses` and `diagnostic_hir_wf_check`. Only the query declarations need to be modified; existing providers and callers continue to work with the new query return type. (My real goal is to apply this to `coverage_ids_info`, which will return Option as of rust-lang#135873, but that PR hasn't landed yet.) [^deref]: Technically they could return other types that implement `Deref`, but it's hard to imagine this working well with anything other than `&T`.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e9e5291): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 2.6%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 777.336s -> 777.188s (-0.02%) |
rustbot
removed
the
S-waiting-on-perf
|
As expected, no measurable impact other than noise, so I'll allow rollup. @bors r=compiler-errors rollup=maybe |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#135073 (Implement `ByteStr` and `ByteString` types) - rust-lang#135492 (Add missing check for async body when suggesting await on futures.) - rust-lang#135766 (handle global trait bounds defining assoc types) - rust-lang#135880 (Get rid of RunCompiler) - rust-lang#135908 (rustc_codegen_llvm: remove outdated asm-to-obj codegen note) - rust-lang#135911 (Allow `arena_cache` queries to return `Option<&'tcx T>`) - rust-lang#135920 (simplify parse_format::Parser::ws by using next_if) r? `@ghost` `@rustbot` modify labels: rollup
Currently,
arena_cachequeries always have to return&'tcx T1. This means that if an arena-cached query wants to return an optional value, it has to return&'tcx Option<T>, which has a few negative consequences:Option<&T>is preferred over&Option<T>.Option<&T>have to manually call.as_ref()on the query result.None, a full-sizedOption<T>still needs to be stored in the arena.This PR solves that problem by introducing a helper trait
ArenaCachedthat is implemented for both&TandOption<&T>, and takes care of bridging between the provided type, the arena-allocated type, and the declared query return type.To demonstrate that this works, I have converted the two existing arena-cached queries that currently return
&Option<T>:mir_coroutine_witnessesanddiagnostic_hir_wf_check. Only the query declarations need to be modified; existing providers and callers continue to work with the new query return type.(My real goal is to apply this to
coverage_ids_info, which will return Option as of #135873, but that PR hasn't landed yet.)Footnotes
Technically they could return other types that implement
Deref, but it's hard to imagine this working well with anything other than&T. ↩