Introduce `ConstAllocation`. #94597

nnethercote · 2022-03-04T05:21:43Z

Currently some Allocations are interned, some are not, and it's very
hard to tell at a use point which is which.

This commit introduces ConstAllocation for the known-interned ones,
which makes the division much clearer. ConstAllocation::inner() is
used to get the underlying Allocation.

In some places it's natural to use an Allocation, in some it's natural
to use a ConstAllocation, and in some places there's no clear choice.
I've tried to make things look as nice as possible, while generally
favouring ConstAllocation, which is the type that embodies more
information. This does require quite a few calls to inner().

The commit also tweaks how PartialOrd works for Interned. The
previous code was too clever by half, building on T: Ord to make the
code shorter. That caused problems with deriving PartialOrd and Ord
for ConstAllocation, so I changed it to build on T: PartialOrd,
which is slightly more verbose but much more standard and avoided the
problems.

r? @fee1-dead

rust-highfive · 2022-03-04T05:21:45Z

Some changes occured to rustc_codegen_gcc

cc @antoyo

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

nnethercote · 2022-03-04T05:22:06Z

This is the next step after #93148.

nnethercote · 2022-03-04T05:41:52Z

I don't expect this to have much effect on performance, but let's check.

@bors try @rust-timer queue

rust-timer · 2022-03-04T05:41:54Z

Awaiting bors try build completion.

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

bors · 2022-03-04T05:42:01Z

⌛ Trying commit 904767d40562bb4e5775232d903c6a654f91cccd with merge 3cf02802d38ab3495d18e703694f862dd416f888...

bors · 2022-03-04T06:54:44Z

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

rust-timer · 2022-03-04T06:54:46Z

Queued 3cf02802d38ab3495d18e703694f862dd416f888 with parent 8fa5d74, future comparison URL.

rust-timer · 2022-03-04T14:24:42Z

Finished benchmarking commit (3cf02802d38ab3495d18e703694f862dd416f888): comparison url.

Summary: This benchmark run shows 15 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant regressions: 0.3%
Arithmetic mean of relevant improvements: -0.9%
Arithmetic mean of all relevant changes: -0.7%
Largest improvement in instruction counts: -1.4% on full builds of keccak check

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.

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

RalfJung · 2022-03-04T15:56:08Z

No objection from my side. I was at first a bit confused since it wasn't clear that ConstAllocation already is a reference (in contrast to Allocation which isn't), but I also do not have good ideas for how to avoid that confusion.

Mark-Simulacrum · 2022-03-04T16:39:25Z

It seems like "const" might be referring to either somehow const-related allocations or constant allocations (i.e., immutable contents) -- I presume that it is the second, right? Maybe we should call it an InternedAllocation or ImmutableAllocation to try to head off some of this potential for confusion?

RalfJung · 2022-03-04T18:19:15Z

It's the first. Some of these allocations will be mutable at runtime, that is controlled by a flag inside Allocation.

Mark-Simulacrum · 2022-03-04T18:48:25Z

Hm. I guess I don't fully understand the distinction, then. The comment on ConstAllocation says nothing about const, for example.

I would expect that runtime mutability is indeed distinct from immutability of the allocation at compile time -- do we only intern top-level allocations (for some definition of top-level, perhaps those associated with a DefId)?

(My opinions here aren't blockers or that strong, of course -- feel free to tell me that the name will be obvious for those working with this part of the compiler often).

nnethercote · 2022-03-04T18:49:52Z

It seems like "const" might be referring to either somehow const-related allocations or constant allocations (i.e., immutable contents) -- I presume that it is the second, right? Maybe we should call it an InternedAllocation or ImmutableAllocation to try to head off some of this potential for confusion?

These were already being called "const allocations" (e.g. pre-existing const_allocation, intern_const_alloc identifiers) so I just went with the existing names. And we generally avoid Interned as a prefix, e.g. types like Ty, Predicate, Region are all interned.

nnethercote · 2022-03-04T18:50:23Z

Nice to see this was a small instruction win for a few cases. Though cycles and wall-time didn't change in any significant way, so maybe it is perf-neutral in practice.

Mark-Simulacrum · 2022-03-04T18:53:10Z

OK -- then since this is already being used elsewhere, seems OK to continue using it for now. We can consider a rename later down the line if it turns out that it is confusing for more than just me :)

fee1-dead

r=me after nit

fee1-dead · 2022-03-05T01:40:19Z

compiler/rustc_middle/src/ty/print/pretty.rs

@@ -1434,7 +1435,7 @@ pub trait PrettyPrinter<'tcx>:
                // The `inspect` here is okay since we checked the bounds, and there are no
                // relocations (we have an active `str` reference here). We don't use this
                // result to affect interpreter execution.
-                let slice = data.inspect_with_uninit_and_ptr_outside_interpreter(start..end);
+                let slice = data.0.inspect_with_uninit_and_ptr_outside_interpreter(start..end);


Perhaps using inner like everywhere else would be more consistent here?

fee1-dead · 2022-03-05T01:41:01Z

@bors delegate+

bors · 2022-03-05T01:41:04Z

✌️ @nnethercote can now approve this pull request

Currently some `Allocation`s are interned, some are not, and it's very hard to tell at a use point which is which. This commit introduces `ConstAllocation` for the known-interned ones, which makes the division much clearer. `ConstAllocation::inner()` is used to get the underlying `Allocation`. In some places it's natural to use an `Allocation`, in some it's natural to use a `ConstAllocation`, and in some places there's no clear choice. I've tried to make things look as nice as possible, while generally favouring `ConstAllocation`, which is the type that embodies more information. This does require quite a few calls to `inner()`. The commit also tweaks how `PartialOrd` works for `Interned`. The previous code was too clever by half, building on `T: Ord` to make the code shorter. That caused problems with deriving `PartialOrd` and `Ord` for `ConstAllocation`, so I changed it to build on `T: PartialOrd`, which is slightly more verbose but much more standard and avoided the problems.

nnethercote · 2022-03-06T21:45:34Z

@bors r=fee1-dead

bors · 2022-03-06T21:45:35Z

📌 Commit 4852291 has been approved by fee1-dead

bors · 2022-03-06T22:37:58Z

⌛ Testing commit 4852291 with merge 8876ca3...

bors · 2022-03-07T01:18:33Z

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing 8876ca3 to master...

rust-highfive · 2022-03-07T01:18:55Z

📣 Toolstate changed by #94597!

Tested on commit 8876ca3.
Direct link to PR: #94597

💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

@oli-obk

Tested on commit rust-lang/rust@8876ca3. Direct link to PR: <rust-lang/rust#94597> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).

rust-timer · 2022-03-07T03:52:43Z

Finished benchmarking commit (8876ca3): comparison url.

Summary: This benchmark run shows 13 relevant improvements 🎉 to instruction counts.

Arithmetic mean of relevant improvements: -0.9%
Arithmetic mean of all relevant changes: -0.8%
Largest improvement in instruction counts: -1.4% on full builds of keccak check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

rust-highfive assigned fee1-dead Mar 4, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 4, 2022

fee1-dead reviewed Mar 5, 2022

View reviewed changes

nnethercote mentioned this pull request Mar 5, 2022

Caching the stable hash of Ty within itself #94299

Merged

fee1-dead 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 Mar 6, 2022

nnethercote force-pushed the ConstAllocation branch from 904767d to 4852291 Compare March 6, 2022 21:45

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 Mar 6, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 7, 2022

bors merged commit 8876ca3 into rust-lang:master Mar 7, 2022

rustbot added this to the 1.61.0 milestone Mar 7, 2022

rust-highfive mentioned this pull request Mar 7, 2022

miri no longer builds after rust-lang/rust#94597 #94687

Closed

bors mentioned this pull request Mar 7, 2022

Back InitMask by IntervalSet #94450

Closed

nnethercote deleted the ConstAllocation branch March 7, 2022 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `ConstAllocation`. #94597

Introduce `ConstAllocation`. #94597

nnethercote commented Mar 4, 2022

rust-highfive commented Mar 4, 2022

nnethercote commented Mar 4, 2022

nnethercote commented Mar 4, 2022

rust-timer commented Mar 4, 2022

bors commented Mar 4, 2022

bors commented Mar 4, 2022

rust-timer commented Mar 4, 2022

rust-timer commented Mar 4, 2022

RalfJung commented Mar 4, 2022

Mark-Simulacrum commented Mar 4, 2022

RalfJung commented Mar 4, 2022

Mark-Simulacrum commented Mar 4, 2022

nnethercote commented Mar 4, 2022 •

edited

Loading

nnethercote commented Mar 4, 2022 •

edited

Loading

Mark-Simulacrum commented Mar 4, 2022

fee1-dead left a comment

fee1-dead Mar 5, 2022

fee1-dead commented Mar 5, 2022

bors commented Mar 5, 2022

nnethercote commented Mar 6, 2022

bors commented Mar 6, 2022

bors commented Mar 6, 2022

bors commented Mar 7, 2022

rust-highfive commented Mar 7, 2022

rust-timer commented Mar 7, 2022

Introduce ConstAllocation. #94597

Introduce ConstAllocation. #94597

Conversation

nnethercote commented Mar 4, 2022

rust-highfive commented Mar 4, 2022

nnethercote commented Mar 4, 2022

nnethercote commented Mar 4, 2022

rust-timer commented Mar 4, 2022

bors commented Mar 4, 2022

bors commented Mar 4, 2022

rust-timer commented Mar 4, 2022

rust-timer commented Mar 4, 2022

RalfJung commented Mar 4, 2022

Mark-Simulacrum commented Mar 4, 2022

RalfJung commented Mar 4, 2022

Mark-Simulacrum commented Mar 4, 2022

nnethercote commented Mar 4, 2022 • edited Loading

nnethercote commented Mar 4, 2022 • edited Loading

Mark-Simulacrum commented Mar 4, 2022

fee1-dead left a comment

Choose a reason for hiding this comment

fee1-dead Mar 5, 2022

Choose a reason for hiding this comment

fee1-dead commented Mar 5, 2022

bors commented Mar 5, 2022

nnethercote commented Mar 6, 2022

bors commented Mar 6, 2022

bors commented Mar 6, 2022

bors commented Mar 7, 2022

rust-highfive commented Mar 7, 2022

rust-timer commented Mar 7, 2022

Introduce `ConstAllocation`. #94597

Introduce `ConstAllocation`. #94597

nnethercote commented Mar 4, 2022 •

edited

Loading

nnethercote commented Mar 4, 2022 •

edited

Loading