Add cache for allocate_str

[adwinwhite]

Best effort cache for string allocation in const eval.

Fixes rust-lang/miri#3470.

[rustbot]

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[oli-obk]

We recreate the InterpCtxt on every constant, so this does not do any cross-constant caching, which is where it is actually needed I believe. This would need to work similar to how we create only a single alloc id for static items.

[adwinwhite]

Oh, I didn't realize InterpCtxt isn't shared.

Does it mean I should put the cache in GlobalCtxt and use TyCtxt::allocate_bytes to allocate global memory for these strings?
AllocId/Pointer is unique globally and InterpCtxt can read global memory so it should work.

[oli-obk]

I think changing allocate_bytes to use https://doc.rust-lang.org/stable/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html#method.reserve_and_set_dedup internally will do exactly what we want, as we already have a cache

[adwinwhite]

This approach looks elegant!

The only problem is that reserve_and_set_dedup doesn't work with GlobalAlloc::Memory(..).
Should we inline an unrestricted version of reserve_and_set_dedup into allocate_bytes by hand or just allow GlobalAlloc::Memory in reserve_and_set_dedup?

[oli-obk]

just allow GlobalAlloc::Memory in reserve_and_set_dedup?

yea, we originally restricted it because we didn't have a use case, so it seems fine to just remove the check and permit it

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[adwinwhite]

Good to know that. I'll remove the kind check then.

…

On Sun, Jul 14, 2024, 14:56 Ralf Jung ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In compiler/rustc_const_eval/src/interpret/place.rs <#127638 (comment)>: > @@ -1001,7 +1001,20 @@ where kind: MemoryKind<M::MemoryKind>, mutbl: Mutability, ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> { - let ptr = self.allocate_bytes_ptr(str.as_bytes(), Align::ONE, kind, mutbl)?; + let tcx = self.tcx.tcx; + + // Use cache for immutable strings. + let ptr = if mutbl.is_not() + && matches!(kind, MemoryKind::CallerLocation | MemoryKind::Machine(_)) Immutable memory can never be deallocated. And anyway memory in the global tcx map can also never be deallocated. — Reply to this email directly, view it on GitHub <#127638 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMRDI4RFQOZYHN34V3HDHS3ZMIOI3AVCNFSM6AAAAABKYIFB2CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZWGU4DSNRTGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[oli-obk]

@bors r+

[bors]

📌 Commit e595f3d has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

@bors r- actually let's perf it first

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit e595f3d with merge b43ab65...

[bors]

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

[rust-timer]

Finished benchmarking commit (b43ab65): comparison URL.

Overall result: ✅ improvements - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 3.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 700.232s -> 700.469s (0.03%)
Artifact size: 328.70 MiB -> 328.66 MiB (-0.01%)

[oli-obk]

@bors rollup-

[bors]

📌 Commit e595f3d has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit e595f3d with merge 5c84886...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5c84886 to master...

[rust-timer]

Finished benchmarking commit (5c84886): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -3.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 700.582s -> 700.173s (-0.06%)
Artifact size: 328.68 MiB -> 328.67 MiB (-0.00%)