-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Rename, clarify, and document code for "erasing" query values #151565
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? @SparrowLii rustbot has assigned @SparrowLii. Use |
| /// Trait for types that can be erased into [`Erased<Self>`]. | ||
| /// | ||
| /// Erasing and unerasing values is performed by [`erase_val`] and [`restore_val`]. | ||
| pub trait Erasable: Copy { |
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.
Erasable goes against the notional convention of naming traits after verbs rather than adjectives, but in this case I find erasable so much clearer that I think it's justified.
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.
certainly can't be worse the status quo, since it wasn't Erase before. what does "EraseType" mean? "you can erase this type"? yeah it implements the trait, doesn't it?
( type Erase simply doesn't make much sense to me. )
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| const { | ||
| if size_of::<T>() != size_of::<T::Result>() { | ||
| if size_of::<T>() != size_of::<T::Storage>() { | ||
| panic!("size of T must match erased type T::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.
| panic!("size of T must match erased type T::Result") | |
| panic!("size of T must match erased type T::Storage") |
| Erased::<<T as EraseType>::Result> { | ||
| ErasedData::<<T as Erasable>::Storage> { | ||
| // `transmute_unchecked` is needed here because it does not have `transmute`'s size check | ||
| // (and thus allows to transmute between `T` and `MaybeUninit<T::Result>`) (we do the size |
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.
| // (and thus allows to transmute between `T` and `MaybeUninit<T::Result>`) (we do the size | |
| // (and thus allows to transmute between `T` and `MaybeUninit<T::Storage>`) (we do the size |
| $( | ||
| impl<'tcx> EraseType for $($fake_path)::+<'tcx> { | ||
| type Result = [u8; size_of::<$($fake_path)::+<'static>>()]; | ||
| impl<'tcx> Erasable for $($fake_path)::+<'tcx> { |
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.
| impl<'tcx> Erasable for $($fake_path)::+<'tcx> { | |
| impl Erasable for $($fake_path)::+<'_> { |
I think this will work for any paths with one lifetime argument at the end, not just 'tcx.
A number of explicit impls from the above can probably be merged into this macro too.
| let key = key.into_query_param(); | ||
| if let Some(res) = try_get_cached(tcx, query_cache, &key) { | ||
| erase::restore(res).map(drop) | ||
| erase::restore_val(res).map(drop) |
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.
Hmm, why drop?
Erasable types are Copy, they cannot have any nontrivial drop.
| queries::$name::Key<'tcx>, | ||
| QueryMode, | ||
| ) -> Option<Erase<$V>>,)* | ||
| ) -> Option<::rustc_middle::query::erase::Erased<$V>>,)* |
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.
| ) -> Option<::rustc_middle::query::erase::Erased<$V>>,)* | |
| ) -> Option<$crate::query::erase::Erased<$V>>,)* |
And two more instances above.
We are in rustc_middle, so this looks a bit weird in declarative macros, but it's a matter of style. Other code above uses $crate though.
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.
I don't find these changes to be useful or clarifying.
| //! To improve compile times and code size for the compiler itself, query | ||
| //! values are "erased" in some contexts (e.g. inside in-memory cache types), | ||
| //! to reduce the number of generic instantiations created during codegen. |
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.
Source of this statement needs clarification. Benchmarks on the original PR (#109333 (comment)) do not seem to have introduced improvements.
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.
Yeah, it would be good to check if it's still necessary.
#108638 says "The compile time of rustc_query_impl is reduced by 27%" which by itself doesn't sound like a convincing reason to complicate the query system, but pictures like #65031 (comment) make it more convincing.
In order to reduce compile times and code size for the compiler itself, the query system has a mechanism for “erasing” and ”restoring” query values in certain contexts. See #109333 for the original implementation.
Unfortunately, the erasure system has very little documentation, and involves a dizzying assortment of similarly-named types, traits, and functions.
This PR therefore renames several parts of the erasure API and implementation to hopefully be clearer, and adds comments to better explain the purpose and mechanism behind value erasure.
Summary of renames:
erase→erase_val(avoiding ambiguity with moduleerase)restore→restore_valErase<T>→Erased<T>(for actual erased values ofT)EraseType→Erasable(for types that can be erased and restored)EraseType::Result→Erasable::StorageErased<T>→ErasedData<Storage>There should be no change to compiler behaviour.