-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Hash ty::Const
using its pointer
#90951
Conversation
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit c08ad47d35c6ef722fdf6479ad67052ad4f9ab1f with merge 43984a53f55c0c3d3071d63a0de7d5ea04454ef1... |
.. do try builds need cg_clif? let's find out.. |
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f8d46ef
to
6194cc8
Compare
@fee1-dead |
If you want to retry a perf run, just |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 6194cc8705cd39e0cd3b79a2795ca8dc1fa19ae3 with merge 2d9c1f2f85f568c21d4eb21c7f5bf5d2e9168ef7... |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit b824865 with merge a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227... |
☀️ Try build successful - checks-actions |
Queued a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227 with parent 6414e0b, future comparison URL. |
Finished benchmarking commit (a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
Thanks for the PR, @fee1-dead! This is a rather large change set and the performance impact seems to be quite ambiguous. Would you say that it also improves code quality? Looking at performance alone I'm don't think it's worth the trouble. |
I would argue that the performance impact isn't ambiguous, since the regression is related to dividing codegen units and opaque stuff that isn't directly caused by this PR. Hashing/Comparing interned pointers is almost always better than looking at the data. This should ideally be done to every interned type. |
Maybe run just the part that makes the fields private through perf so we get it independently of the hashing and equality changes? |
In general that makes sense to me. I'll take a closer look tomorrow. |
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.
OK, I took a closer look. Thanks again for the PR, @fee1-dead!
It looks correct to me except for the missing equality impls for &'tcx Const<'tcx>
. Can you add those?
In general (and this is already the case before this PR) I think it is rather fishy that we don't have a proper smart pointer type for interned things and instead rely on &'tcx Foo<'tcx>
to probably do the right thing and people adding custom impls around that.
|
||
impl<'tcx> Eq for Interned<'tcx, Const<'tcx>> {} | ||
|
||
#[repr(transparent)] |
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.
Please add a comment that #[repr(transparent)]
is needed because of the Borrow impl below.
let c: &'tcx Const<'tcx> = *self; | ||
(c as *const Const<'tcx>).hash(state) | ||
} | ||
} |
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.
Shouldn't there be impl PartialEq for &'tcx Const<'tcx>
and impl Eq for &'tcx Const<'tcx>
that follow the same pattern too? Otherwise the HashMap will do a deep equality check during insertion, right?
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.
AIUI this isn't necessary because library/core/src/cmp.rs provides impl<A: ?Sized, B: ?Sized> PartialEq<&B> for &A
...
|
||
#[repr(transparent)] | ||
#[derive(Clone, Copy)] | ||
struct CstHash<'tcx>(Const<'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.
Maybe CstHash
could be renamed to something more expressive, like ConstInterningKey
or something.
|
||
pub val: ConstKind<'tcx>, | ||
pub(super) val: ConstKind<'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.
Please add a comment that the fields have restricted visibility because we want to prevent Const
from being constructed in a non-interned way.
ty::Const implements Copy and Clone. |
Indeed! That's a potential correctness problem. Can these impls be removed? |
Ping from triage: |
#93148 is a better approach; Thus this is obsolete |
The idea comes from the last commit of rust-lang#90951.
ty::Const
is always interned, so using its pointer for hashing and equality always works.By setting fields private and using getter functions instead of accessing the fields directly, we guarantee that a
ty::Const
is never constructed without being interned.