Hash `ty::Const` using its pointer #90951

fee1-dead · 2021-11-16T16:09:16Z

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.

rust-highfive · 2021-11-16T16:09:19Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-11-16T16:09:20Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

fee1-dead · 2021-11-16T16:11:29Z

@bors try @rust-timer queue

rust-timer · 2021-11-16T16:11:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-16T16:11:38Z

⌛ Trying commit c08ad47d35c6ef722fdf6479ad67052ad4f9ab1f with merge 43984a53f55c0c3d3071d63a0de7d5ea04454ef1...

fee1-dead · 2021-11-16T16:20:25Z

.. do try builds need cg_clif? let's find out..

bors · 2021-11-16T16:27:35Z

💔 Test failed - checks-actions

matthiaskrgr · 2021-11-16T17:12:37Z

@fee1-dead @bors retry is for retrying to merge already approved PRS that failed in ci. It does NOT "retry" a @bors try build. try and retry are for different things.
@bors r-

matthiaskrgr · 2021-11-16T17:13:15Z

If you want to retry a perf run, just
@bors try @rust-timer queue
again

rust-timer · 2021-11-16T17:13:16Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-16T17:13:24Z

⌛ Trying commit 6194cc8705cd39e0cd3b79a2795ca8dc1fa19ae3 with merge 2d9c1f2f85f568c21d4eb21c7f5bf5d2e9168ef7...

fee1-dead · 2021-11-18T14:43:13Z

@bors try @rust-timer queue

rust-timer · 2021-11-18T14:43:15Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-18T14:43:23Z

⌛ Trying commit b824865 with merge a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227...

bors · 2021-11-18T16:19:39Z

☀️ Try build successful - checks-actions
Build commit: a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227 (a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227)

rust-timer · 2021-11-18T16:19:41Z

Queued a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227 with parent 6414e0b, future comparison URL.

rust-timer · 2021-11-18T17:51:47Z

Finished benchmarking commit (a99a0ac3f4ccc1aaa8d59194fc97e793be2cc227): comparison url.

Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.

Small improvement in instruction counts (up to -0.7% on incr-full builds of ctfe-stress-4)
Moderate regression in instruction counts (up to 0.7% on full builds of helloworld)

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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

fee1-dead · 2021-11-24T13:10:53Z

See Zulip (archive) about the perf regression

michaelwoerister · 2021-11-25T14:08:21Z

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.

fee1-dead · 2021-11-30T07:02:05Z

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.

oli-obk · 2021-11-30T11:52:35Z

Maybe run just the part that makes the fields private through perf so we get it independently of the hashing and equality changes?

michaelwoerister · 2021-12-02T15:21:13Z

Hashing/Comparing interned pointers is almost always better than looking at the data. This should ideally be done to every interned type.

In general that makes sense to me. I'll take a closer look tomorrow.

michaelwoerister

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.

michaelwoerister · 2021-12-03T10:26:22Z

compiler/rustc_middle/src/ty/context.rs

+
+impl<'tcx> Eq for Interned<'tcx, Const<'tcx>> {}
+
+#[repr(transparent)]


Please add a comment that #[repr(transparent)] is needed because of the Borrow impl below.

michaelwoerister · 2021-12-03T10:44:19Z

compiler/rustc_middle/src/ty/consts.rs

+        let c: &'tcx Const<'tcx> = *self;
+        (c as *const Const<'tcx>).hash(state)
+    }
+}


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?

AIUI this isn't necessary because library/core/src/cmp.rs provides impl<A: ?Sized, B: ?Sized> PartialEq<&B> for &A...

michaelwoerister · 2021-12-03T10:51:42Z

compiler/rustc_middle/src/ty/context.rs

+
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+struct CstHash<'tcx>(Const<'tcx>);


Maybe CstHash could be renamed to something more expressive, like ConstInterningKey or something.

michaelwoerister · 2021-12-03T11:00:31Z

compiler/rustc_middle/src/ty/consts.rs


-    pub val: ConstKind<'tcx>,
+    pub(super) val: ConstKind<'tcx>,


Please add a comment that the fields have restricted visibility because we want to prevent Const from being constructed in a non-interned way.

tmiasko · 2021-12-06T12:54:46Z

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.

ty::Const implements Copy and Clone.

michaelwoerister · 2021-12-14T09:27:53Z

ty::Const implements Copy and Clone.

Indeed! That's a potential correctness problem. Can these impls be removed?

JohnCSimon · 2022-01-23T01:14:47Z

Ping from triage:
@fee1-dead can you post your status on this PR?

fee1-dead · 2022-01-23T06:05:34Z

#93148 is a better approach; Thus this is obsolete

The idea comes from the last commit of rust-lang#90951.

rust-highfive assigned michaelwoerister Nov 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2021

fee1-dead force-pushed the iref branch from e41908d to c08ad47 Compare November 16, 2021 16:11

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 16, 2021

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2021

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the iref branch from c08ad47 to 37375c3 Compare November 16, 2021 16:39

This comment has been minimized.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2021

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the iref branch 2 times, most recently from f8d46ef to 6194cc8 Compare November 16, 2021 17:08

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 18, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 18, 2021

michaelwoerister reviewed Dec 3, 2021

View reviewed changes

michaelwoerister added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2021

nnethercote mentioned this pull request Jan 21, 2022

Overhaul interning. #93148

Merged

fee1-dead closed this Jan 23, 2022

fee1-dead deleted the iref branch January 28, 2022 10:58

nnethercote mentioned this pull request Feb 21, 2022

Make Const::val() return a ref, to avoid copies. #94202

Closed

nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 21, 2022

Make Const::val() return a ref, to avoid copies.

a9d3cd1

The idea comes from the last commit of rust-lang#90951.


		impl<'tcx> Eq for Interned<'tcx, Const<'tcx>> {}

		#[repr(transparent)]

Hash ty::Const using its pointer #90951

Hash ty::Const using its pointer #90951

Conversation

fee1-dead commented Nov 16, 2021

rust-highfive commented Nov 16, 2021

rust-highfive commented Nov 16, 2021

fee1-dead commented Nov 16, 2021

rust-timer commented Nov 16, 2021

bors commented Nov 16, 2021

fee1-dead commented Nov 16, 2021

bors commented Nov 16, 2021

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.

matthiaskrgr commented Nov 16, 2021 • edited Loading

matthiaskrgr commented Nov 16, 2021

rust-timer commented Nov 16, 2021

bors commented Nov 16, 2021

This comment has been minimized.

fee1-dead commented Nov 18, 2021

rust-timer commented Nov 18, 2021

bors commented Nov 18, 2021

bors commented Nov 18, 2021

rust-timer commented Nov 18, 2021

rust-timer commented Nov 18, 2021

fee1-dead commented Nov 24, 2021

michaelwoerister commented Nov 25, 2021

fee1-dead commented Nov 30, 2021 • edited Loading

oli-obk commented Nov 30, 2021

michaelwoerister commented Dec 2, 2021

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Dec 3, 2021

Choose a reason for hiding this comment

michaelwoerister Dec 3, 2021

Choose a reason for hiding this comment

nnethercote Jan 28, 2022

Choose a reason for hiding this comment

michaelwoerister Dec 3, 2021

Choose a reason for hiding this comment

michaelwoerister Dec 3, 2021

Choose a reason for hiding this comment

tmiasko commented Dec 6, 2021

michaelwoerister commented Dec 14, 2021

JohnCSimon commented Jan 23, 2022

fee1-dead commented Jan 23, 2022

Hash `ty::Const` using its pointer #90951

Hash `ty::Const` using its pointer #90951

matthiaskrgr commented Nov 16, 2021 •

edited

Loading

fee1-dead commented Nov 30, 2021 •

edited

Loading