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.

Sign up for GitHub

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

Jump to bottom

Enable effects for libcore #114776

Merged
merged 6 commits into from Sep 22, 2023
Merged

Enable effects for libcore #114776

merged 6 commits into from Sep 22, 2023

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Aug 13, 2023 •
edited

r? @oli-obk

forgot you are on vacation, oops

@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2023

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 13, 2023
@rustbot

This comment was marked as off-topic.

Sign in to view
fee1-dead
fee1-dead commented Aug 13, 2023
View reviewed changes
compiler/rustc_hir/src/lang_items.rs Outdated
@@ -227,7 +227,7 @@ language_item_table! {
// in the sense that a crate is not required to have it defined to use it, but a final product
// is required to define it somewhere. Additionally, there are restrictions on crates that use
// a weak lang item, but do not have it defined.
Panic, sym::panic, panic_fn, Target::Fn, GenericRequirement::Exact(0);
Panic, sym::panic, panic_fn, Target::Fn, GenericRequirement::Minimum(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

should change this to Exact(1) here

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fee1-dead
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 13, 2023

⌛ Trying commit f7c0ba5 with merge fb627bbd863c7a76a25668de0676cab2598a8946...

@bors
Copy link
Contributor

bors commented Aug 13, 2023

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

@fee1-dead
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-114776 created and queued.
🤖 Automatically detected try build fb627bbd863c7a76a25668de0676cab2598a8946
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@fee1-dead fee1-dead marked this pull request as draft August 13, 2023 12:33
@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-114776 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Skepfyr
Copy link
Contributor

Skepfyr commented Aug 13, 2023

FYI for the crater run, this looks like it has a commit that is probably causing new ICEs, see #114783. The ICEs caused by that issue should be reasonably easy to identify though.

@fee1-dead
Copy link
Member Author

I think crater runs compare rustc without this PR with rustc with this PR, so I'm assuming they would classify "ICEs without PR and also ICEs with PR" in its own category.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-114776 is completed!
📊 42 regressed and 0 fixed (347798 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 14, 2023
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Sep 12, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 20, 2023

⌛ Trying commit 2063ebc with merge 9305cef...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2023
@bors
Auto merge of rust-lang#114776 - fee1-dead-contrib:enable-effects-in-…
9305cef 
…libcore, r=<try>

Enable effects for libcore

~~r? `@oli-obk~~`

forgot you are on vacation, oops
@bors
Copy link
Contributor

bors commented Sep 20, 2023

☀️ Try build successful - checks-actions
Build commit: 9305cef (9305cef68ff30c6eb197ffaca8c71af554fd053a)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9305cef): 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-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.

mean range count
Regressions ❌
(primary)		 0.3% [0.2%, 0.3%] 5
Regressions ❌
(secondary)		 0.7% [0.3%, 0.9%] 3
Improvements ✅
(primary)		 -4.0% [-4.0%, -4.0%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.4% [-4.0%, 0.3%] 6

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.

mean range count
Regressions ❌
(primary)		 2.1% [0.8%, 3.8%] 5
Regressions ❌
(secondary)		 2.9% [2.6%, 3.1%] 3
Improvements ✅
(primary)		 -1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.4% [-1.7%, 3.8%] 6

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.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -3.4% [-3.4%, -3.4%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -3.4% [-3.4%, -3.4%] 1

Binary size

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.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.4%] 25
Regressions ❌
(secondary)		 0.1% [0.0%, 0.1%] 15
Improvements ✅
(primary)		 -0.3% [-0.4%, -0.1%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.4%] 30

Bootstrap: 632.622s -> 631.912s (-0.11%)
Artifact size: 317.96 MiB -> 317.99 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 Sep 20, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Sep 20, 2023

The perf run shows a few extra executions of queries and metadata encoding, which seems unavoidable to me. We may get the perf back when ripping out the const checker's logic that is only needed until we enable effects by default for doing const fn callability checks

@rustbot
Copy link
Collaborator

rustbot commented Sep 22, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@oli-obk
Copy link
Contributor

oli-obk commented Sep 22, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2023

📌 Commit ab168b6 has been approved by oli-obk

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 Sep 22, 2023
@bors
Copy link
Contributor

bors commented Sep 22, 2023

⌛ Testing commit ab168b6 with merge 8759de0...

@bors
Copy link
Contributor

bors commented Sep 22, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8759de0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2023
@bors bors merged commit 8759de0 into rust-lang:master Sep 22, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 22, 2023
@bors bors mentioned this pull request Sep 22, 2023
Closed
@fee1-dead fee1-dead deleted the enable-effects-in-libcore branch September 22, 2023 09:45
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8759de0): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

mean range count
Regressions ❌
(primary)		 0.3% [0.3%, 0.4%] 4
Regressions ❌
(secondary)		 0.6% [0.3%, 0.8%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.4% [-0.5%, -0.3%] 4
All ❌✅ (primary) 0.3% [0.3%, 0.4%] 4

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.

mean range count
Regressions ❌
(primary)		 2.4% [2.4%, 2.4%] 1
Regressions ❌
(secondary)		 2.8% [1.6%, 4.0%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.0% [-1.2%, -0.9%] 2
All ❌✅ (primary) 2.4% [2.4%, 2.4%] 1

Cycles

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

Binary size

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.

mean range count
Regressions ❌
(primary)		 0.1% [0.0%, 0.4%] 26
Regressions ❌
(secondary)		 0.1% [0.0%, 0.1%] 14
Improvements ✅
(primary)		 -0.3% [-0.4%, -0.1%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [-0.4%, 0.4%] 28

Bootstrap: 632.249s -> 634.614s (0.37%)
Artifact size: 317.50 MiB -> 317.63 MiB (0.04%)

@fee1-dead
Copy link
Member Author

per this comment by oli

@rustbot label perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 22, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 25, 2023
@bors
Auto merge of rust-lang#114776 - fee1-dead-contrib:enable-effects-in-…
84917c8 
…libcore, r=oli-obk

Enable effects for libcore

~~r? `@oli-obk~~`

forgot you are on vacation, oops
@lcnr lcnr removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Sep 29, 2023
@oli-obk oli-obk mentioned this pull request Oct 17, 2023
Merged
aliemjay added a commit to aliemjay/rust that referenced this pull request Oct 18, 2023
@aliemjay
Rollup merge of rust-lang#116856 - oli-obk:no_effects, r=compiler-errors
a536d58 
Disable effects in libcore again

r? `@fee1-dead`

This was accidentally allowed by rust-lang#114776 without feature gates
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 18, 2023
@rust-timer
Unrolled build for rust-lang#116856
309a65e 
Rollup merge of rust-lang#116856 - oli-obk:no_effects, r=compiler-errors

Disable effects in libcore again

r? `@fee1-dead`

This was accidentally allowed by rust-lang#114776 without feature gates
@fee1-dead fee1-dead mentioned this pull request Nov 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
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 Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@fee1-dead @rustbot @rust-log-analyzer @bors @craterbot @Skepfyr @oli-obk @rust-timer @cjgillot @lcnr