Skip to content

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Jul 23, 2025

This PR used to tweak the implementation of impl_of_method, but that introduced a perf regression.

Rename impl_of_method and trait_of_item to impl_of_assoc and trait_of_assoc respectively. This reflects how the two functions are closely related. And it reflects the behavior more accurately as the functions check whether the input is an associated item.

@rustbot
Copy link
Collaborator

rustbot commented Jul 23, 2025

r? @fee1-dead

rustbot has assigned @fee1-dead.
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 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 Jul 23, 2025
@rust-log-analyzer

This comment has been minimized.

@camsteffen camsteffen force-pushed the simplify-impl-of-method branch from 230e8b9 to e019fd7 Compare July 23, 2025 21:04
@Kobzol
Copy link
Member

Kobzol commented Jul 24, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Jul 24, 2025

⌛ Trying commit e019fd7 with merge bf4a373

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Jul 24, 2025
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 24, 2025
@fee1-dead
Copy link
Member

fee1-dead commented Jul 24, 2025

this should really be renamed to opt_impl_parent or impl_of_assoc instead.

r=me after good/neutral perf, rename can be in followup

@rust-bors
Copy link

rust-bors bot commented Jul 24, 2025

☀️ Try build successful (CI)
Build commit: bf4a373 (bf4a373133f6ff486e137e70291a5d9e95fe7fae, parent: 3c30dbbe31bfbf6029f4534170165ba573ff0fd1)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bf4a373): comparison URL.

Overall result: ❌ regressions - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@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.4% [0.1%, 0.7%] 17
Regressions ❌
(secondary)
0.3% [0.0%, 0.6%] 24
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [0.1%, 0.7%] 17

Max RSS (memory usage)

Results (primary -1.4%, secondary -1.8%)

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)
-1.4% [-2.3%, -0.9%] 4
Improvements ✅
(secondary)
-1.8% [-2.8%, -1.2%] 4
All ❌✅ (primary) -1.4% [-2.3%, -0.9%] 4

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: 468.586s -> 466.337s (-0.48%)
Artifact size: 374.68 MiB -> 374.63 MiB (-0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 24, 2025
@camsteffen
Copy link
Contributor Author

Hmm that doesn't look like a win. I guess it's because the non assoc input case is now parent+def_kind where it used to be just def_kind. There could be an opportunity to split out a panicking version but I'm not sure if that's worth it either.

Agreed about the rename.

@fee1-dead
Copy link
Member

Well, i'd be happy to r+ if you repurpose this PR so that it's just the renaming :)

@camsteffen camsteffen force-pushed the simplify-impl-of-method branch from e019fd7 to 448868a Compare July 25, 2025 00:10
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. PG-exploit-mitigations Project group: Exploit mitigations T-clippy Relevant to the Clippy team. labels Jul 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred to constck

cc @fee1-dead

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

HIR ty lowering was modified

cc @fmease

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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 in compiler/rustc_sanitizers

cc @rcvalle

@camsteffen camsteffen changed the title Simplify impl_of_method Rename impl_of_method and trait_of_item Jul 25, 2025
@camsteffen
Copy link
Contributor Author

Oooof that's a lot of tags 🫣

@fee1-dead
Copy link
Member

still potentially perf sensitive, so

@bors2 try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Jul 25, 2025

⌛ Trying commit 448868a with merge 0ed9f77

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Jul 25, 2025
@fee1-dead
Copy link
Member

r=me after rebase

@bors delegate+

@bors
Copy link
Collaborator

bors commented Jul 28, 2025

✌️ @camsteffen, you can now approve this pull request!

If @fee1-dead told you to "r=me" after making some further change, please make that change, then do @bors r=@fee1-dead

@camsteffen camsteffen force-pushed the simplify-impl-of-method branch from f257f7b to cdcfdd1 Compare July 28, 2025 14:55
@camsteffen
Copy link
Contributor Author

@bors r=fee1-dead

@bors
Copy link
Collaborator

bors commented Jul 28, 2025

📌 Commit cdcfdd1 has been approved by fee1-dead

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 Jul 28, 2025
@bors
Copy link
Collaborator

bors commented Jul 28, 2025

⌛ Testing commit cdcfdd1 with merge e3514bd...

@bors
Copy link
Collaborator

bors commented Jul 28, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2025
@bors bors merged commit e3514bd into rust-lang:master Jul 28, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 28, 2025
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 9ba00e0 (parent) -> e3514bd (this PR)

Test differences

Show 10 test diffs

10 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 e3514bde96d2d13586337a48db77fa64b850d249 --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. x86_64-apple-1: 6510.5s -> 8257.3s (26.8%)
  2. dist-aarch64-apple: 6553.4s -> 5064.3s (-22.7%)
  3. dist-apple-various: 7336.1s -> 5799.0s (-21.0%)
  4. aarch64-apple: 5219.9s -> 6249.7s (19.7%)
  5. dist-x86_64-apple: 11486.0s -> 9607.8s (-16.4%)
  6. aarch64-gnu-llvm-19-2: 2285.8s -> 2647.3s (15.8%)
  7. pr-check-2: 2267.7s -> 2617.6s (15.4%)
  8. x86_64-apple-2: 4118.0s -> 4752.7s (15.4%)
  9. x86_64-gnu-llvm-19: 2450.6s -> 2809.7s (14.7%)
  10. x86_64-rust-for-linux: 2624.6s -> 2956.9s (12.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 (e3514bd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -4.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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.0% [-4.0%, -4.0%] 1
All ❌✅ (primary) - - 0

Cycles

Results (primary -0.4%)

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

mean range count
Regressions ❌
(primary)
3.3% [3.3%, 3.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-2.5%, -2.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-2.5%, 3.3%] 3

Binary size

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

Bootstrap: 468.487s -> 468.526s (0.01%)
Artifact size: 376.81 MiB -> 376.81 MiB (0.00%)

tautschnig added a commit to tautschnig/kani that referenced this pull request Jul 29, 2025
Relevant upstream PRs:
- rust-lang/rust#144389 (MIR-build: No longer
  emit assumes in enum-as casting)
- rust-lang/rust#144392 (rustc_public: Remove
  movability from `RigidTy/AggregateKind::Coroutine`)
- rust-lang/rust#144377 (Rename impl_of_method
  and trait_of_item)

Resolves: model-checking#4246
github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Jul 29, 2025
Relevant upstream PRs:
- rust-lang/rust#144389 (MIR-build: No longer
emit assumes in enum-as casting)
- rust-lang/rust#144392 (rustc_public: Remove
movability from `RigidTy/AggregateKind::Coroutine`)
- rust-lang/rust#144377 (Rename impl_of_method
and trait_of_item)

Resolves: #4246

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
sagudev added a commit to sagudev/servo that referenced this pull request Sep 24, 2025
sagudev added a commit to sagudev/servo that referenced this pull request Sep 25, 2025
@camsteffen camsteffen deleted the simplify-impl-of-method branch October 9, 2025 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. merged-by-bors This PR was explicitly merged by bors. PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-clippy Relevant to the Clippy team. 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.

7 participants