-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Querify FnAbi::of_{fn_ptr,instance}
as fn_abi_of_{fn_ptr,instance}
.
#88575
Conversation
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri Some changes occured to rustc_codegen_cranelift cc @bjorn3 Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
Thanks to #88499 (comment) (i.e. #88499 being perf-neutral), we should be able to get useful data despite this being based on #88499: @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit c4b36be083fe7fcc74e824b1e15368094d58fbf8 with merge eb6a18b59b113d9f9fde80bfc8c1e43dca27c954... |
☀️ Try build successful - checks-actions |
Queued eb6a18b59b113d9f9fde80bfc8c1e43dca27c954 with parent 50171c3, future comparison URL. |
Yeah, it's on my list... |
Finished benchmarking try commit (eb6a18b59b113d9f9fde80bfc8c1e43dca27c954): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking 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 led to changes in compiler perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @bors rollup=never |
Unfortunately this is a clear perf regression. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8966c5c
to
417f2d1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2b1cf0e
to
d50b4a2
Compare
d50b4a2
to
8c918d7
Compare
Yea, imo we can merge this if we have an issue for the incremental-unchanged regression that somewhat collects the information from this discussion |
+1, seems ok to merge to me. |
@bors r+ rollup=never |
📌 Commit 8c918d7 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9119882): comparison url. Summary: This change led to moderate relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Visited for weekly performance triage. Results do indeed seem moderately mixed. |
@pnkfelix It's a bit more subtle than that (only |
Note: opening this PR as draft because it's based on #88499
This more or less replicates the
LayoutOf::layout_of
setup from #88499, to replaceFnAbi::of_{fn_ptr,instance}
withFnAbiOf::fn_abi_of_{fn_ptr,instance}
, and also route them through queries (whichlayout_of
has used for a while).The two changes at the use sites (other than the names) are:
&'tcx
extra_args
list is now an interned&'tcx ty::List<Ty<'tcx>>
Theoretically, a
FnAbiOfHelpers
implementer could choose to keep theResult<...>
instead of eagerly erroring, but the only existing users of these APIs are codegen backends, so they don't (want to) take advantage of this.At least miri could make use of this, since it prefers propagating errors (it "just" doesn't use
FnAbi
yet - cc @RalfJung).The way this is done is probably less efficient than what is possible, because the queries handle the correctness-oriented API (i.e. the split into
fn
pointers vs instances), whereas a lower-level query could end up with more reuse between different instances with identical signatures.r? @nagisa cc @oli-obk @bjorn3