Skip to content

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Mar 31, 2025

@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 Mar 31, 2025
@lcnr
Copy link
Contributor Author

lcnr commented Mar 31, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2025
@bors
Copy link
Collaborator

bors commented Mar 31, 2025

⌛ Trying commit e57808d with merge b6303fd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2025
stupid fix for coercion hack perf regression

cc rust-lang#136127 rust-lang#138542

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2025
more interesting fix for coercion hack perf regression

cc rust-lang#136127 rust-lang#138542 rust-lang#139171

r? `@ghost`
@bors
Copy link
Collaborator

bors commented Mar 31, 2025

☀️ Try build successful - checks-actions
Build commit: b6303fd (b6303fdafa669e76fca4cabfa942c07295d3f457)

@rust-timer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Apr 1, 2025

@rust-timer build b6303fd

1 similar comment
@compiler-errors
Copy link
Member

@rust-timer build b6303fd

@compiler-errors
Copy link
Member

Oh, this is the perf results: https://perf.rust-lang.org/compare.html?start=10a76d634781180b4f5402c519f0c237d3be6ee6&end=b6303fdafa669e76fca4cabfa942c07295d3f457

That's why rust-timer isn't requeueing the job, b/c it's already done

@lcnr
Copy link
Contributor Author

lcnr commented Apr 1, 2025

it seems like the perf results are pretty much equivalent between this and #138542. Would prefer going ahead with this PR @compiler-errors

it has a better vibe for me. I think the big part is avoiding to use may_coerce in the happy path and keeping casts as

  • try coerce
  • if that fails, cast

@compiler-errors
Copy link
Member

I'm still mostly in favor of the approach to relocate a (solely) cast-related hack to casting code, which is the primary motivation of #138542.

I know that you seem to have a negative opinion of the added complexity of the casting code in that PR, but IMO that added complexity (which is now bubbled up into check_cast) reflects the complexity of the change being introduced in #138542 and its interaction with the strangely (possibly load bearing) false-positive-biased behavior of coerce_unsized.

There's quite a few usages of coercion around HIR typeck, and none of them care about *const dyn Tr -> *const W<dyn Tr>-like coercions other than as casts, so I don't see why we wouldn't want to localize this behavior as closely to the code that concerns it to avoid its behavioral side-effects even needing to be considered for other types of coercion.

Making coerce_unsized less complicated is definitely a benefit that this PR doesn't really have, even if it recovers most of the perf loss in the original PR.

@lcnr
Copy link
Contributor Author

lcnr commented Jun 15, 2025

closing in favor of #138542

@lcnr lcnr closed this Jun 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-perf Status: Waiting on a perf run to be completed. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants