-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Make some load-from-disk function pointers optional in query vtables #151736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Make some load-from-disk function pointers optional in query vtables
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ce4f469): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.9%, secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.1%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 472.873s -> 473.545s (0.14%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of questions below.
| query_state: std::mem::offset_of!(QueryStates<'tcx>, $name), | ||
| query_cache: std::mem::offset_of!(QueryCaches<'tcx>, $name), | ||
| cache_on_disk: |tcx, key| ::rustc_middle::query::cached::$name(tcx, key), | ||
| will_cache_on_disk_for_key_fn: should_ever_cache_on_disk!([$($modifiers)*] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you've added a should_ever_cache_on_disk! call. Was that missing previously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it looks like should_ever_cache_on_disk! now returns Some/None at all use sites. I think it could be simplified accordingly, to look like hash_result!.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you've added a
should_ever_cache_on_disk!call. Was that missing previously?
Yes. Previously, for queries that don't cache to disk, rustc_macros would generate a stub function that always returns false. With this PR, rustc_macros doesn't generate that function at all for non-disk-cached queries, and we use should_ever_cache_on_disk! to avoid needing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it looks like
should_ever_cache_on_disk!now returnsSome/Noneat all use sites. I think it could be simplified accordingly, to look likehash_result!.
I find it easier to think of should_ever_cache_on_disk! as a compile-time if expression, with the two arms expanding to whatever makes sense at the call site.
So I think moving Some/None into the macro would be a false simplification; it's better to have them clearly visible at the call site.
|
@bors r+ rollup=iffy |
|
I expect I will probably have to rebase this after #151666. |
|
☔ The latest upstream changes (presumably #151778) made this pull request unmergeable. Please resolve the merge conflicts. This pull request was unapproved. |
For queries that never incremental-cache to disk, we can represent that fact with None (i.e. a null function pointer) in their vtables, and avoid having to generate stub functions that do nothing.
(There is no decrease in vtable size; we just go from 3/8 padding bytes to 4/8 padding bytes.)
There should be no change to compiler output.