Skip to content
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

Add default trait implementations for "c-unwind" ABI function pointers #101263

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Add default trait implementations for "c-unwind" ABI function pointers #101263

merged 4 commits into from
Oct 21, 2022

Conversation

lopopolo
Copy link
Contributor

@lopopolo lopopolo commented Sep 1, 2022

Following up on #92964, only add default trait implementations for the c-unwind family of function pointers. The previous attempt in #92964 added trait implementations for many more ABIs and ran into concerns regarding the increase in size of the libcore rlib.

An attempt to abstract away function pointer types behind a unified trait to reduce the duplication of trait impls is being discussed in #99531 but this change looks to be blocked on a lang MCP.

Following @RalfJung's suggestion in #99531 (comment), this commit is another cut at #92964 but it only adds the impls for extern "C-unwind" fn and unsafe extern "C-unwind" fn.

I am interested in landing this patch to unblock the stabilization of the c_unwind feature.

RFC: rust-lang/rfcs#2945
Tracking Issue: #74990

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 1, 2022
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2022
@lopopolo
Copy link
Contributor Author

lopopolo commented Sep 1, 2022

it looks like I need to bless changes to the UI tests. Let me try and figure out how to do that 😄

Edit: done!

@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 1, 2022
@bors
Copy link
Contributor

bors commented Sep 1, 2022

⌛ Trying commit 2f7cc2d574b203e1f19b63ee705fe217fefc31ee with merge c97600b8b2ec5ef84e1426b75f8be57bf5450712...

@bors
Copy link
Contributor

bors commented Sep 1, 2022

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

@rust-timer
Copy link
Collaborator

Queued c97600b8b2ec5ef84e1426b75f8be57bf5450712 with parent aa857eb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c97600b8b2ec5ef84e1426b75f8be57bf5450712): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
1.0% [0.3%, 2.6%] 14
Regressions ❌
(secondary)
1.9% [0.6%, 2.7%] 22
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.1% [-1.4%, -0.8%] 6
All ❌✅ (primary) 1.0% [0.3%, 2.6%] 14

Max RSS (memory usage)

Results

This 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.

mean1 range count2
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
2.9% [1.5%, 4.3%] 9
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Cycles

Results

This 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.

mean1 range count2
Regressions ❌
(primary)
2.3% [2.0%, 2.8%] 3
Regressions ❌
(secondary)
2.8% [2.2%, 4.5%] 10
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.3% [2.0%, 2.8%] 3

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 1, 2022
@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2022

Yeah, as expected that extra metadata in the rlib makes everything measurably slower. Not sure if there is a way that we can avoid some of these instances (do we really need all the traits here), but in the end it boils down to judging whether that's a cost we are willing to accept for the benefit of #74990.

Sounds like a @rust-lang/compiler question to me.

@RalfJung RalfJung added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Sep 1, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Sep 8, 2022

Discussed in T-compiler triage meeting.

I think we should move forward with this, despite the performance hit. I don't want to block c-unwind based on the performance regression observed above.

(If it turns out that the performance impact matters, then we can adopt other approaches, such as the one suggested in #99531; though I'm hoping that will not be necessary solely for this issue.)

@rustbot label: -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Sep 8, 2022
@pnkfelix
Copy link
Member

pnkfelix commented Sep 8, 2022

(also, as a side note: the primary benchmarks only regressed on "doc full" scenarios. Normal compilation (check/debug/opt) all came out looking great I would say.)

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 8, 2022
@lopopolo
Copy link
Contributor Author

lopopolo commented Sep 8, 2022

@RalfJung Given the above triage on the perf regression from the compiler team, what are the next steps for getting this approved and merged?

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2022

It needs someone from t-libs to review and approve.
r? libs

@rust-highfive rust-highfive assigned thomcc and unassigned m-ou-se Sep 8, 2022
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impl looks fine, I think this needs libs-api signoff, or perhaps an ACP.

@thomcc thomcc removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 9, 2022
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2022
Following up on #92964, only add default trait implementations for the
`c-unwind` family of function pointers. The previous attempt in #92964
added trait implementations for many more ABIs and ran into concerns
regarding the increase in size of the libcore rlib.

An attempt to abstract away function pointer types behind a unified
trait to reduce the duplication of trait impls is being discussed in #99531
but this change looks to be blocked on a lang MCP.

Following @RalfJung's suggestion in
#99531 (comment),
this commit is another cut at #92964 but it _only_ adds the impls for
`extern "C-unwind" fn` and `unsafe extern "C-unwind" fn`.

I am interested in landing this patch to unblock the stabilization of
the `c_unwind` feature.

RFC: rust-lang/rfcs#2945
Tracking Issue: #74990
@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 20, 2022
@rustbot

This comment was marked as outdated.

@lopopolo
Copy link
Contributor Author

I couldn't figure out the right combination of --stage and --keep-stage flags to pass to x.py to check this locally, but I did rebase to include #103239 and reworked the macro from my proposed patch above. Let's see if CI passes.

@lopopolo
Copy link
Contributor Author

lopopolo commented Oct 20, 2022

Looks like the build still fails with an #[unstable] annotation here has no effect error. @m-ou-se do I need to wait for a new bootstrap compiler to be released?

@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 20, 2022

Looks like the build still fails with an #[unstable] annotation here has no effect error. @m-ou-se do I need to wait for a new bootstrap compiler to be released?

Oh. :( Yeah. I suppose you could apply #[cfg(not(bootstrap))] to line 1868. These trait impls aren't used in the compiler, meaning it's fine if they're missing from the first stage std.

These need to wait until #103239 makes it into the bootstrap compiler.
@lopopolo
Copy link
Contributor Author

@thomcc I've made the stability annotation changes. This is ready for review.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 20, 2022
@thomcc
Copy link
Member

thomcc commented Oct 20, 2022

LGTM, thanks.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2022

📌 Commit efe61da has been approved by thomcc

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 20, 2022
@bors
Copy link
Contributor

bors commented Oct 21, 2022

⌛ Testing commit efe61da with merge 5c8bff7...

@bors
Copy link
Contributor

bors commented Oct 21, 2022

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 5c8bff7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 21, 2022
@bors bors merged commit 5c8bff7 into rust-lang:master Oct 21, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 21, 2022
@lopopolo lopopolo deleted the lopopolo/c-unwind-fn-ptr-impls branch October 21, 2022 23:47
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5c8bff7): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
0.9% [0.2%, 2.6%] 19
Regressions ❌
(secondary)
1.9% [0.4%, 3.0%] 23
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.2%, 2.6%] 19

Max RSS (memory usage)

Results

This 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.

mean1 range count2
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
2.8% [1.5%, 4.9%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) 1.4% [1.4%, 1.4%] 1

Cycles

Results

This 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.

mean1 range count2
Regressions ❌
(primary)
2.5% [2.1%, 3.0%] 2
Regressions ❌
(secondary)
2.7% [2.1%, 3.1%] 13
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.1%, 3.0%] 2

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@nnethercote
Copy link
Contributor

It's all doc perf regressions, and was decided earlier to be acceptable.

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.