Temporarily bring back `Rvalue::Len` #135709

lqd · 2025-01-18T22:16:45Z

r? @compiler-errors as requested in #135671 (comment)

However, in the mean time, I'd rather we not crunch trying to find and more importantly validate the soundness of a solution 🤔

Agreed. To fix the IMO P-critical #135671 for which we somehow didn't have test coverage, this PR temporarily reverts:

cc @scottmcm

I added the few samples from that issue as a test, but we can add more in the future, in particular it seems @steffahn will work on that.

Fixes #135671. And if we want to land this, it should also be nominated for beta backport.

… r=matthewjasper" This reverts commit e108481, reversing changes made to 303e8bd.

…-obk" This reverts commit 7c301ec, reversing changes made to dffaad8.

…trmetadata, r=davidtwco,RalfJung" This reverts commit b57d93d, reversing changes made to 0aeaa5e.

rustbot · 2025-01-18T22:16:56Z

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

compiler-errors · 2025-01-18T23:39:02Z

cc @scottmcm @RalfJung

Let me know if y'all would like to brainstorm how to fix this properly, and let's maybe do another crater run before fully removing the operand again. Crater runs should be really quick these days. I think the right solution would be to emit some sort of special copy operation to clone the slice pointer but allow borrowck and miri to understand that we're only copying the ref to read its metadata, similarly to how we have CopyForDeref for deref'ing which has special semantics. Or perhaps augment the RawRef rvalue operand to pass along the fact that it should act like a fake borrow.

Sorry that the test coverage was lacking; I could not have expected anyone to catch this without a failing test, but thank goodness we have beta backports :)

I'll leave this open for a day or so b/c it's the weekend, but I'll likely r+ this soon thereafter. Also open to just backporting this and trying to find a fix-forward on master, though that comes with all the normal complications of that approach that I think we're all aware of.

scottmcm · 2025-01-19T01:13:43Z

No worries from my end about reverting these. Definitely better to get the regression fixed.

I can take a couple of simpler smaller steps on the way too, like getting Len out of runtime MIR while still using it for borrowck, while we figure out how best to represent this in earlier phases.

Glad to hear there's a plan to get more tests here!

compiler-errors · 2025-01-19T04:19:27Z

well if scott is ok, then ill r+ it now

@bors r+ rollup=never

@rustbot label: +beta-nominated

bors · 2025-01-19T04:19:30Z

📌 Commit c69dea9 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2025-01-19T06:09:55Z

⌛ Testing commit c69dea9 with merge c62b732...

bors · 2025-01-19T08:49:03Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing c62b732 to master...

rust-timer · 2025-01-19T10:06:40Z

Finished benchmarking commit (c62b732): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	0.4%	[0.2%, 0.5%]	6
Improvements ✅ (primary)	-0.7%	[-1.3%, -0.4%]	3
Improvements ✅ (secondary)	-0.9%	[-1.7%, -0.2%]	2
All ❌✅ (primary)	-0.4%	[-1.3%, 0.3%]	4

Max RSS (memory usage)

Results (primary -2.3%, secondary -1.6%)

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)	5.6%	[5.6%, 5.6%]	1
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-3.2%	[-11.9%, -0.7%]	9
Improvements ✅ (secondary)	-2.9%	[-3.7%, -2.1%]	3
All ❌✅ (primary)	-2.3%	[-11.9%, 5.6%]	10

Cycles

Results (primary -1.1%, secondary 1.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)	4.2%	[4.0%, 4.6%]	3
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-2.0%	[-2.2%, -1.9%]	2
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Binary size

Results (primary 0.0%, secondary 0.3%)

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.2%	[0.0%, 0.6%]	18
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-0.1%	[-0.7%, -0.0%]	16
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.7%, 0.6%]	34

Bootstrap: 765.901s -> 765.96s (0.01%)
Artifact size: 326.07 MiB -> 325.96 MiB (-0.04%)

lqd · 2025-01-19T10:31:38Z

Revert

@rustbot label: +perf-regression-triaged

RalfJung · 2025-01-19T16:58:23Z

I think the right solution would be to emit some sort of special copy operation to clone the slice pointer but allow borrowck and miri to understand that we're only copying the ref to read its metadata, similarly to how we have CopyForDeref for deref'ing which has special semantics. Or perhaps augment the RawRef rvalue operand to pass along the fact that it should act like a fake borrow.

Those sound like promising approaches. Adding this info to Rvalue::RawRef does seem a bit hacky, but if both borrowck and Miri need this info then maybe it's not so bad after all? I hope Miri can stop needing this in the future, but that needs more work -- ironically also related to s.len() calls; specifically we need a way for the implicit borrow of s here to not lead to a retag.

lqd added 4 commits January 18, 2025 22:09

Revert "Auto merge of rust-lang#134330 - scottmcm:no-more-rvalue-len,…

ca1c17c

… r=matthewjasper" This reverts commit e108481, reversing changes made to 303e8bd.

Revert "Rollup merge of rust-lang#134371 - scottmcm:fix-134352, r=oli…

0bb4880

…-obk" This reverts commit 7c301ec, reversing changes made to dffaad8.

Revert "Auto merge of rust-lang#133734 - scottmcm:lower-indexing-to-p…

122a55b

…trmetadata, r=davidtwco,RalfJung" This reverts commit b57d93d, reversing changes made to 0aeaa5e.

add tests for issue 135671

c69dea9

rustbot assigned compiler-errors Jan 18, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 18, 2025

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2025

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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 19, 2025

bors merged commit c62b732 into rust-lang:master Jan 19, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 19, 2025

bors mentioned this pull request Jan 19, 2025

#[contracts::requires(...)] + #[contracts::ensures(...)] #128045

Open

lqd deleted the bring-back-len branch January 19, 2025 09:23

rustbot added the perf-regression Performance regression. label Jan 19, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporarily bring back `Rvalue::Len` #135709

Temporarily bring back `Rvalue::Len` #135709

lqd commented Jan 18, 2025

rustbot commented Jan 18, 2025

compiler-errors commented Jan 18, 2025 •

edited

Loading

scottmcm commented Jan 19, 2025 •

edited

Loading

compiler-errors commented Jan 19, 2025

bors commented Jan 19, 2025

bors commented Jan 19, 2025

bors commented Jan 19, 2025

rust-timer commented Jan 19, 2025

lqd commented Jan 19, 2025

RalfJung commented Jan 19, 2025

Temporarily bring back Rvalue::Len #135709

Temporarily bring back Rvalue::Len #135709

Conversation

lqd commented Jan 18, 2025

rustbot commented Jan 18, 2025

compiler-errors commented Jan 18, 2025 • edited Loading

scottmcm commented Jan 19, 2025 • edited Loading

compiler-errors commented Jan 19, 2025

bors commented Jan 19, 2025

bors commented Jan 19, 2025

bors commented Jan 19, 2025

rust-timer commented Jan 19, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lqd commented Jan 19, 2025

RalfJung commented Jan 19, 2025

Temporarily bring back `Rvalue::Len` #135709

Temporarily bring back `Rvalue::Len` #135709

compiler-errors commented Jan 18, 2025 •

edited

Loading

scottmcm commented Jan 19, 2025 •

edited

Loading