-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Deref
args when #[const_trait]
is enabled
#118386
Conversation
@rustbot author |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #118282) made this pull request unmergeable. Please resolve the merge conflicts. |
af3ca15
to
a40e51c
Compare
This comment has been minimized.
This comment has been minimized.
This code is still broken for two reasons:
|
a40e51c
to
38e0402
Compare
This is ready :> We just need to pass the host param to @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Fix `Deref` args when `#[const_trait]` is enabled r? `@fee1-dead` This is still pretty broken, but putting this PR up so I don't lose the code in some `git stash` somewhere...
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (4a90a2c): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.958s -> 673.623s (-0.05%) |
I feel like we just need to accept this perf regression 😅, since it's likely not possible to avoid needing to pass in the host param in method_autoderef_steps, and we're simply doing more work here. Happy to discuss this if anyone disagrees, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry about the lack of response here, r=me after the two tiny nits
tests/ui/rfcs/rfc-2632-const-trait-impl/effects/const_closure-const_trait_impl-ice-113381.rs
Outdated
Show resolved
Hide resolved
38e0402
to
a453884
Compare
I'm curious about perf... @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Fix `Deref` args when `#[const_trait]` is enabled Fix `Deref` being a `#[const_trait]`. To fix this fully, we also need to pass the host param to `method_autoderef_steps` and make sure we use it correctly in `Autoderef`.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (34aeaa2): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 667.831s -> 668.134s (0.05%) |
I think there may be potential optimizations related to caching behavior with the host generic params. Currently the trait system would probably cache non-const trait bounds and const trait bounds separately. If we find a way to mark which host args come from the original obligation carried forward, then we'd only need to select traits once (considering that concrete impls like I'm not entirely sure why we'd have this many regressions, though. |
Well, hmm, it looks like |
I think it may just be due to additional caching. I will test a few things, but otherwise I feel like we should absorb the perf hit since it seems inevitable. |
feel free to r=me after you have done the testing |
This is gonna have to be reworked after @fee1-dead adjusts the const trait desugaring, I think. Or otherwise, this can be revived later. |
Fix
Deref
being a#[const_trait]
. To fix this fully, we also need to pass the host param tomethod_autoderef_steps
and make sure we use it correctly inAutoderef
.