Skip to content

Conversation

@dianne
Copy link
Contributor

@dianne dianne commented Sep 27, 2025

When lowering non-overloaded indexing operations to MIR, this uses the temporary lifetime of the index expression for the index temporary, rather than applying the temporary lifetime of the indexing operation as a whole to the index.

For example, in

let x = &xs[i];

previously, the temporary containing the result of evaluating i would live until the end of the block due to the indexing operation being lifetime-extended. Under this PR, the index temporary only lives to the end of the let statement because it uses the more precise temporary lifetime of the index expression.

I don't think this will affect semantics in an observable way, but the more precise StorageDead placement may slightly improve analysis/codegen performance.

r? mir

When lowering non-overloaded indexing operations to MIR, this uses the
temporary lifetime of the index expression for the index, rather than
applying the temporary lifetime of the indexing operation as a whole to
the index.
@rustbot 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. labels Sep 27, 2025
@traviscross traviscross added the I-lang-radar Items that are on lang's radar and will need eventual work or consideration. label Sep 27, 2025
@dianne
Copy link
Contributor Author

dianne commented Oct 23, 2025

Let's see if this actually helps with perf on its own. My hope is that it accounts for the regression in #146098, but there's a chance it does something alone. @bors try @rust-timer queue

Also, since the exact lifetime used for the index is a bit subtle, I feel I should justify it. I think this still doesn't quite match the reference spec: since the index isn't a place expression context, I think destructors.scope.operands would apply rather than using the index expression's temporary scope, but this doesn't quite work for MIR semantics / implementation detail reasons. Per that rule, as I understand it, the temporary holding the index would be associated with the indexing operator expression and dropped after evaluating it. However, the Place representing xs[i] holds the temporary's Local in it, so the temporary holding i has to live until xs[i] is referenced/used, e.g. by Rvalue::Ref or Rvalue::Use from a parent expression/statement. Using the temporary scope of the indexed expression should allow the temporary to live long enough for that, I think: no expressions corresponding to PlaceElems introduce terminating scopes, and if they did, that would have caused problems for the current implementation that uses the indexing operator expression's temporary scope. Since this only applies to built-in array/slice indexing, the temporary living a bit longer than specified shouldn't be observable, I think.

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Oct 23, 2025
Do not lifetime-extend array/slice indices
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 23, 2025
@rust-timer

This comment has been minimized.

@dianne
Copy link
Contributor Author

dianne commented Oct 23, 2025

Writing this in a separate message rather than editing so I don't re-request the perf run again (oops): a more precise (but more involved) alternative to this could be to add a scope argument to Builder::expr_as_place, to be used for indices' scopes. That way the index could be dropped right after evaluating whichever expression/statement references/uses the xs[i] place.

@rust-bors
Copy link

rust-bors bot commented Oct 23, 2025

☀️ Try build successful (CI)
Build commit: 566fcb8 (566fcb89f4c9284b4072877aea113e03b1bc065e, parent: 6244effd0372d5d88fc859d3bf17ce1efcc2c9ec)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (566fcb8): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

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

Cycles

Results (secondary -3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

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

Bootstrap: 471.983s -> 473.334s (0.29%)
Artifact size: 390.70 MiB -> 390.73 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 23, 2025
@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 27, 2025

📌 Commit c0dc979 has been approved by matthewjasper

It is now in the queue for this repository.

@bors 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 Oct 27, 2025
@bors
Copy link
Collaborator

bors commented Oct 28, 2025

⌛ Testing commit c0dc979 with merge df984ed...

@bors
Copy link
Collaborator

bors commented Oct 28, 2025

☀️ Test successful - checks-actions
Approved by: matthewjasper
Pushing df984ed to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 28, 2025
@bors bors merged commit df984ed into rust-lang:master Oct 28, 2025
12 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Oct 28, 2025
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing adaa838 (parent) -> df984ed (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard df984edf44203c862e01b5a20c8092d5614d872e --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-x86_64-apple: 8586.7s -> 6634.9s (-22.7%)
  2. dist-x86_64-msvc-alt: 9799.1s -> 12006.4s (22.5%)
  3. aarch64-apple: 10380.2s -> 8044.7s (-22.5%)
  4. dist-apple-various: 3018.4s -> 3639.4s (20.6%)
  5. x86_64-gnu-gcc: 2836.0s -> 3389.3s (19.5%)
  6. dist-aarch64-apple: 8934.0s -> 7592.0s (-15.0%)
  7. i686-gnu-nopt-1: 7428.2s -> 8359.5s (12.5%)
  8. x86_64-msvc-2: 6390.4s -> 7191.4s (12.5%)
  9. x86_64-gnu-llvm-20: 2188.9s -> 2457.3s (12.3%)
  10. aarch64-gnu-llvm-20-2: 2238.8s -> 2500.4s (11.7%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (df984ed): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.3%, -0.2%] 3
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary 3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (primary -2.6%, secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.8%, 3.0%] 2
Improvements ✅
(primary)
-2.6% [-3.5%, -2.2%] 3
Improvements ✅
(secondary)
-4.0% [-5.6%, -2.4%] 2
All ❌✅ (primary) -2.6% [-3.5%, -2.2%] 3

Binary size

Results (secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 473.263s -> 474.956s (0.36%)
Artifact size: 390.54 MiB -> 390.61 MiB (0.02%)

@dianne dianne deleted the non-extended-indices branch October 28, 2025 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I-lang-radar Items that are on lang's radar and will need eventual work or consideration. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants