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

Move generic NonZero rustc_layout_scalar_valid_range_start attribute to inner type. #121885

Merged
merged 14 commits into from
Mar 17, 2024
Merged

Move generic NonZero rustc_layout_scalar_valid_range_start attribute to inner type. #121885

merged 14 commits into from
Mar 17, 2024

Conversation

reitermarkus
Copy link
Contributor

Tracking issue: #120257

r? @dtolnay

@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 Mar 2, 2024
@reitermarkus reitermarkus mentioned this pull request Mar 2, 2024
Closed
21 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@reitermarkus
Copy link
Contributor Author

@Nilstrieb, @oli-obk, I re-added the rustc_nonnull_optimization_guaranteed to the generic type and highlighted the resulting bug in the lint.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@reitermarkus reitermarkus force-pushed the generic-nonzero-inner branch 2 times, most recently from f93f0ae to 70bf864 Compare March 2, 2024 16:59
@reitermarkus reitermarkus marked this pull request as ready for review March 2, 2024 17:00
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@reitermarkus
Copy link
Contributor Author

@oli-obk, @Nilstrieb, this should be working now.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 5, 2024

r? @oli-obk

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2024

📌 Commit a58eb80 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 15, 2024

📌 Commit 96431e4 has been approved by oli-obk,wesleywiser

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 Mar 15, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@bors
Auto merge of rust-lang#121885 - reitermarkus:generic-nonzero-inner, …
4f99ab5 
…r=oli-obk,wesleywiser

Move generic `NonZero` `rustc_layout_scalar_valid_range_start` attribute to inner type.

Tracking issue: rust-lang#120257

r? `@dtolnay`
@bors
Copy link
Contributor

bors commented Mar 15, 2024

⌛ Testing commit 96431e4 with merge 4f99ab5...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 15, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Mar 16, 2024

@bors r=oli-obk,wesleywiser

@bors
Copy link
Contributor

bors commented Mar 16, 2024

📌 Commit e4b27a2 has been approved by oli-obk,wesleywiser

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 Mar 16, 2024
@bors
Copy link
Contributor

bors commented Mar 17, 2024

⌛ Testing commit e4b27a2 with merge a615cea...

@bors
Copy link
Contributor

bors commented Mar 17, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk,wesleywiser
Pushing a615cea to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 17, 2024
@bors bors merged commit a615cea into rust-lang:master Mar 17, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 17, 2024
This was referenced Mar 17, 2024
Merged
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a615cea): 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.4% [0.3%, 0.6%] 9
Regressions ❌
(secondary)		 0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) 0.4% [0.3%, 0.6%] 9

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)		 6.5% [3.5%, 9.5%] 2
Regressions ❌
(secondary)		 4.8% [4.8%, 4.8%] 1
Improvements ✅
(primary)		 -3.9% [-7.2%, -0.2%] 5
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.9% [-7.2%, 9.5%] 7

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)		 - - 0
Improvements ✅
(secondary)		 -6.5% [-8.1%, -3.7%] 4
All ❌✅ (primary) - - 0

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.5%] 17
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.2% [-0.4%, -0.0%] 2
Improvements ✅
(secondary)		 -0.3% [-0.4%, -0.3%] 3
All ❌✅ (primary) 0.1% [-0.4%, 0.5%] 19

Bootstrap: 669.642s -> 669.054s (-0.09%)
Artifact size: 312.75 MiB -> 312.82 MiB (0.02%)

@rustbot rustbot added the perf-regression Performance regression. label Mar 17, 2024
@reitermarkus reitermarkus deleted the generic-nonzero-inner branch March 17, 2024 14:59
@Kobzol
Copy link
Contributor

Kobzol commented Mar 19, 2024

This looks like a genuine, albeit small, regression. Was it expected that this might regress compilation time?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 19, 2024

Not really, but I guess the extra struct field indirection does have an impact after all. We've seen similar things before when we added another wrapper type inside Box. I think we'll just have to take this hit for now

@Kobzol
Copy link
Contributor

Kobzol commented Mar 19, 2024

Ok, marking as triaged.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk Awaiting requested review from oli-obk

@Noratrieb Noratrieb Awaiting requested review from Noratrieb

@wesleywiser wesleywiser Awaiting requested review from wesleywiser

Assignees

@wesleywiser wesleywiser

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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.79.0
Development

Successfully merging this pull request may close these issues.