Rollup of 8 pull requests #120187

Nadrieril · 2024-01-21T05:38:39Z

Successful merges:

Implement strict integer operations that panic on overflow #116090 (Implement strict integer operations that panic on overflow)
Use bool instead of PartiolOrd as return value of the comparison closure in {slice,Iteraotr}::is_sorted_by #118811 (Use bool instead of PartiolOrd as return value of the comparison closure in {slice,Iteraotr}::is_sorted_by)
Add Ipv6Addr::is_ipv4_mapped #119081 (Add Ipv6Addr::is_ipv4_mapped)
Use an interpreter in MIR jump threading #119461 (Use an interpreter in MIR jump threading)
Move OS String implementation into sys #119996 (Move OS String implementation into sys)
coverage: Format all coverage tests with rustfmt #120015 (coverage: Format all coverage tests with rustfmt)
pattern_analysis: Remove Ty: Copy bound #120027 (pattern_analysis: Remove Ty: Copy bound)
fix(rust-analyzer): use new pkgid spec to compare #120084 (fix(rust-analyzer): use new pkgid spec to compare)

r? @ghost
@rustbot modify labels: rollup

This change consists of cherry-picking the content from the original PR[1], which got closed due to inactivity, and applying the following changes: * Resolving merge conflicts (obviously) * Linked to to_ipv4_mapped instead of to_ipv4 in the documentation (seems more appropriate) * Added the must_use and rustc_const_unstable attributes the original didn't have I think it's a reasonably useful method. [1] rust-lang#86490

These tests deliberately use non-standard formatting, so that the line execution counts reported by `llvm-cov` reveal additional information about where code regions begin and end.

Some of these tests use non-standard formatting that we can simulate by strategically adding `//` line comments. One contains `where` clauses that would be split across multiple lines, which we can keep on one line by moving the bounds to the generic type instead.

These tests can simply be reformatted as normal, because the resulting changes are unimportant.

Starting from cargo#13311, Cargo's compiler artifact message uses Package ID specification as package's identifier format.

…u-se Implement strict integer operations that panic on overflow This PR implements the first part of the ACP for adding panic on overflow style arithmetic operations (rust-lang/libs-team#270), mentioned in rust-lang#116064. It adds the following operations on both signed and unsigned integers: - `strict_add` - `strict_sub` - `strict_mul` - `strict_div` - `strict_div_euclid` - `strict_rem` - `strict_rem_euclid` - `strict_neg` - `strict_shl` - `strict_shr` - `strict_pow` Additionally, signed integers have: - `strict_add_unsigned` - `strict_sub_unsigned` - `strict_abs` And unsigned integers have: - `strict_add_signed` The `div` and `rem` operations are the same as normal division and remainder but are added for completeness similar to the corresponding `wrapping_*` operations. I'm not sure if I missed any operations, I basically found them from the `wrapping_*` and `checked_*` operations on both integer types.

…Simulacrum Use `bool` instead of `PartiolOrd` as return value of the comparison closure in `{slice,Iteraotr}::is_sorted_by` Changes the function signature of the closure given to `{slice,Iteraotr}::is_sorted_by` to return a `bool` instead of a `PartiolOrd` as suggested by the libs-api team here: rust-lang#53485 (comment). This means these functions now return true if the closure returns true for all the pairs of values.

Add Ipv6Addr::is_ipv4_mapped This change consists of cherry-picking the content from the original PR[1], which got closed due to inactivity, and applying the following changes: * Resolving merge conflicts (obviously) * Linked to to_ipv4_mapped instead of to_ipv4 in the documentation (seems more appropriate) * Added the must_use and rustc_const_unstable attributes the original didn't have I think it's a reasonably useful method to have. [1] rust-lang#86490

…tmiasko Use an interpreter in MIR jump threading This allows to understand assignments of aggregate constants. This case appears more frequently with GVN promoting aggregates to constants.

Move OS String implementation into `sys` Part of rust-lang#117276. The new structure is really useful here, since we can easily eliminate a number of ugly `#[path]`-based imports. In the future, it might be good to move the WTF-8 implementation directly to the OS string implementation, I cannot see it being used anywhere else. That is a story for another PR, however.

coverage: Format all coverage tests with `rustfmt` As suggested by <rust-lang#119984 (comment)>. Test files in `tests/` are normally ignored by `x fmt`, but sometimes those files end up being run through `rustfmt` anyway, either by `rust-analyzer` or by hand. When that happens, it's annoying to have to manually revert formatting changes that are unrelated to the actual changes being made. So it's helpful for the tests in the repository to already have standard formatting beforehand. However, there are several coverage tests that deliberately use non-standard formatting, so that line counts reveal more information about where code regions begin and end. In those cases, we can use `#[rustfmt::skip]` to prevent that code from being disturbed. ``@rustbot`` label +A-code-coverage

…compiler-errors pattern_analysis: Remove `Ty: Copy` bound To make it compatible with rust-analyzer's `Ty` which isn't `Copy` (it's an `Arc`). r? ``@compiler-errors``

…acrum fix(rust-analyzer): use new pkgid spec to compare Starting from rust-lang/cargo#13311, Cargo's compiler artifact message uses Package ID specification as package's identifier format. Zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/proc-macro-test.20bootstrap.20and.20pkgid.20JSON cc `@ehuss`

bors · 2024-01-21T09:18:52Z

⌛ Testing commit 01669d2 with merge cb25c5b...

bors · 2024-01-21T11:15:32Z

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing cb25c5b to master...

bors · 2024-01-21T11:15:32Z

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing cb25c5b to master...

rust-timer · 2024-01-21T11:17:48Z

📌 Perf builds for each rolled up PR:

PR#	Message	Perf Build Sha
#116090	Implement strict integer operations that panic on overflow	`363814c8317ee2dc48738276878e86a459eacd01` (link)
#118811	Use `bool` instead of `PartiolOrd` as return value of the c…	`564a5fbaea9377acaf35379a973da9110a97460c` (link)
#119081	Add Ipv6Addr::is_ipv4_mapped	`6fd96aac79d502e1a469e53ead8531d37f45253e` (link)
#119461	Use an interpreter in MIR jump threading	`e96cc852ee1dfa4a7c7a27effd45b4b18d6f5d06` (link)
#119996	Move OS String implementation into `sys`	`0f778c8fc564b029403f60a61684e92aa76fc85b` (link)
#120015	coverage: Format all coverage tests with `rustfmt`	`38febacc36223ee6833b4c483d26492d98b44abc` (link)
#120027	pattern_analysis: Remove `Ty: Copy` bound	`75fedd204e166bb53dba3daf09962f19967849a2` (link)
#120084	fix(rust-analyzer): use new pkgid spec to compare	`e1ef891b56b6f261c69ef28bbcf357c07cd79f84` (link)

previous master: 867d39cdf6

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

rust-timer · 2024-01-21T12:30:06Z

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.3%, 1.0%]	8
Regressions ❌ (secondary)	0.9%	[0.3%, 1.8%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.3%, 1.0%]	8

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)	1.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	3.1%	[2.5%, 3.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.6%, 1.6%]	1

Cycles

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

Binary size

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

Bootstrap: 662.887s -> 662.48s (-0.06%)
Artifact size: 308.36 MiB -> 308.40 MiB (0.01%)

Kobzol · 2024-01-21T12:35:02Z

Hmm, no idea what PR could cause this. Let's go one by one (unless someone has an idea).

@rust-timer build 0f778c8

Kobzol · 2024-01-21T14:11:28Z

@rust-timer build e96cc85

Nadrieril · 2024-01-21T16:21:04Z

Looking through them I'd guess it's #119461 because it affects MIR opts. I probably shouldn't have rolled it up. Mayyybe #118811 or #120027. Can't imagine it's any of the others.

rust-timer · 2024-01-21T16:25:41Z

Finished benchmarking commit (e96cc85): comparison URL.

Overall result: ❌ regressions - no action needed

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
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 662.887s -> 662.256s (-0.10%)
Artifact size: 308.36 MiB -> 308.35 MiB (-0.00%)

Nadrieril · 2024-01-21T16:37:50Z

Doesn't explain everything. #120027 would be a likely culprit but we checked it for perf before the r+. Let's see #118811.

@rust-timer build 564a5fb

rust-timer · 2024-01-21T18:52:46Z

Finished benchmarking commit (564a5fb): comparison URL.

Overall result: ❌ regressions - no action needed

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
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	3.6%	[3.6%, 3.6%]	1
Regressions ❌ (secondary)	2.6%	[2.5%, 2.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.6%	[3.6%, 3.6%]	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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Bootstrap: 662.887s -> 661.092s (-0.27%)
Artifact size: 308.36 MiB -> 308.36 MiB (-0.00%)

Nadrieril · 2024-01-21T20:05:18Z

The rest of the regressions are on documentation, could it be #116090?

@rust-timer build 363814c

rust-timer · 2024-01-21T21:18:56Z

Finished benchmarking commit (363814c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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.5%	[0.3%, 1.1%]	9
Regressions ❌ (secondary)	0.8%	[0.3%, 1.2%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.3%, 1.1%]	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)	-	-	0
Regressions ❌ (secondary)	2.4%	[2.3%, 2.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.5%	[0.5%, 0.6%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.5%, 0.6%]	3

Bootstrap: 662.887s -> 663.187s (0.05%)
Artifact size: 308.36 MiB -> 308.30 MiB (-0.02%)

Nadrieril · 2024-01-21T22:03:52Z

Yup, that's it. In summary, the overall rollup perf impact is explained by:

Use an interpreter in MIR jump threading #119461 does this i.e. regresses deep-vector (secondary bench) by ~1.5%
Use bool instead of PartiolOrd as return value of the comparison closure in {slice,Iteraotr}::is_sorted_by #118811 does this i.e. regresses deep-vector (secondary bench) by ~1.8%
Implement strict integer operations that panic on overflow #116090 does this i.e. regresses documentation time of a bunch of crates by a small amount.

That was my first rollup, it seems I wasn't careful enough about the rollup=maybe cases :')

Kobzol · 2024-01-22T07:48:28Z

@Nadrieril Thank you for the detailed analysis!

Kobzol · 2024-01-23T08:21:33Z

The two regressions on deep-vector seem to be noise, as the benchmark has been oscillating recently. The doc regressions are real, but they are simply caused by adding a bunch of new methods to the standard library.

@rustbot label: +perf-regression-triaged

rmehri01 and others added 23 commits November 24, 2023 11:45

implement strict_* operations for signed integer types

27f419b

implement strict_* operations for unsigned integer types

fb349a2

add #[track_caller] to improve panic locations

8c7a5b0

reword panic comments and add spaces to unlikely branches

6d17169

std: move OS String implementation into sys

70b0364

coverage: Add #[rustfmt::skip] to tests with non-standard formatting

f149442

These tests deliberately use non-standard formatting, so that the line execution counts reported by `llvm-cov` reveal additional information about where code regions begin and end.

coverage: Format all remaining tests

99797bb

These tests can simply be reformatted as normal, because the resulting changes are unimportant.

fix(rust-analyzer): use new pkgid spec to compare

9650c30

Starting from cargo#13311, Cargo's compiler artifact message uses Package ID specification as package's identifier format.

Use an interpreter in jump threading.

be9668d

Extract process_constant.

b22742e

Extract process_assign.

e72b2b1

Remove Ty: Copy bound

796cdc5

Use bool instead of PartiolOrd in is_sorted_by

606eeb8

Rollup merge of rust-lang#119461 - cjgillot:jump-threading-interp, r=…

203cc69

…tmiasko Use an interpreter in MIR jump threading This allows to understand assignments of aggregate constants. This case appears more frequently with GVN promoting aggregates to constants.

Rollup merge of rust-lang#120027 - Nadrieril:remove-ty-copy-bound, r=…

e7f3dc7

…compiler-errors pattern_analysis: Remove `Ty: Copy` bound To make it compatible with rust-analyzer's `Ty` which isn't `Copy` (it's an `Arc`). r? ``@compiler-errors``

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 Jan 21, 2024

bors added merged-by-bors This PR was explicitly merged by bors. labels Jan 21, 2024

bors merged commit cb25c5b into rust-lang:master Jan 21, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 21, 2024

rustbot added the perf-regression Performance regression. label Jan 21, 2024

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

Nadrieril deleted the rollup-xfwrb0c branch January 21, 2024 22:03

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup of 8 pull requests #120187

Rollup of 8 pull requests #120187

Nadrieril commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-timer commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Kobzol commented Jan 21, 2024

This comment has been minimized.

This comment was marked as outdated.

Kobzol commented Jan 21, 2024

This comment has been minimized.

Nadrieril commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Nadrieril commented Jan 21, 2024

This comment has been minimized.

rust-timer commented Jan 21, 2024

Nadrieril commented Jan 21, 2024

This comment has been minimized.

rust-timer commented Jan 21, 2024

Nadrieril commented Jan 21, 2024

Kobzol commented Jan 22, 2024

Kobzol commented Jan 23, 2024

Rollup of 8 pull requests #120187

Rollup of 8 pull requests #120187

Conversation

Nadrieril commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-timer commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Overall result: ❌ regressions - ACTION NEEDED

Kobzol commented Jan 21, 2024

This comment has been minimized.

This comment was marked as outdated.

Kobzol commented Jan 21, 2024

This comment has been minimized.

Nadrieril commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Overall result: ❌ regressions - no action needed

Nadrieril commented Jan 21, 2024

This comment has been minimized.

rust-timer commented Jan 21, 2024

Overall result: ❌ regressions - no action needed

Nadrieril commented Jan 21, 2024

This comment has been minimized.

rust-timer commented Jan 21, 2024

Overall result: ❌ regressions - ACTION NEEDED

Nadrieril commented Jan 21, 2024

Kobzol commented Jan 22, 2024

Kobzol commented Jan 23, 2024