Reduce integer `Display` implementation size #133247

GuillaumeGomez · 2024-11-20T13:35:38Z

I was thinking about #128204 and how we could reduce the size of the code and just realized that we didn't need the _fmt method to be implemented on signed integers, which in turns allow to simplify greatly the macro call.

r? @workingjubilee

library/core/src/fmt/num.rs

workingjubilee · 2024-11-20T20:14:32Z

Thanks! r=me with squash.

GuillaumeGomez · 2024-11-20T23:04:45Z

@bors r=workingjubilee

bors · 2024-11-20T23:04:48Z

📌 Commit a991f25 has been approved by workingjubilee

It is now in the queue for this repository.

…lay-impl, r=workingjubilee Reduce integer `Display` implementation size I was thinking about rust-lang#128204 and how we could reduce the size of the code and just realized that we didn't need the `_fmt` method to be implemented on signed integers, which in turns allow to simplify greatly the macro call. r? `@workingjubilee`

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#131736 (Emscripten: link with -sWASM_BIGINT) - rust-lang#132207 (Store resolution for self and crate root module segments) - rust-lang#133153 (Add visits to nodes that already have flat_maps in ast::MutVisitor) - rust-lang#133218 (Implement `~const` item bounds in RPIT) - rust-lang#133228 (Rewrite `show_md_content_with_pager`) - rust-lang#133247 (Reduce integer `Display` implementation size) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-11-21T06:55:46Z

@bors r-
guess this failed here? #133273 (comment)

GuillaumeGomez · 2024-11-21T13:32:10Z

@bors r=workingjubilee

bors · 2024-11-21T13:32:13Z

📌 Commit ab38c6d has been approved by workingjubilee

It is now in the queue for this repository.

…lay-impl, r=workingjubilee Reduce integer `Display` implementation size I was thinking about rust-lang#128204 and how we could reduce the size of the code and just realized that we didn't need the `_fmt` method to be implemented on signed integers, which in turns allow to simplify greatly the macro call. r? `@workingjubilee`

workingjubilee · 2024-11-22T02:10:53Z

? PR CI doesn't run library tests...?

workingjubilee · 2024-11-22T02:16:15Z

@bors r-

workingjubilee · 2024-11-22T02:32:51Z

library/core/src/fmt/num.rs

        #[cfg(feature = "optimize_for_size")]
        fn $gen_name(mut n: $u, is_nonnegative: bool, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-            // 2^128 is about 3*10^38, so 39 gives an extra byte of space
-            let mut buf = [MaybeUninit::<u8>::uninit(); 39];
+            const SIZE: usize = $u::MAX.ilog(10) as usize + 1;
+            let mut buf = [MaybeUninit::<u8>::uninit(); SIZE];


? What is the effect of this change on binary size? Or is $u always the same?

On smaller targets, the maximum size might be of u32 and not u64 so since we're there... But on the binary size, there should be no difference for this change.

I see, hopefully so.

…mpiler-errors Rollup of 8 pull requests Successful merges: - rust-lang#131505 (use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin) - rust-lang#131859 (Update TRPL to add new Chapter 17: Async and Await) - rust-lang#132090 (Stop being so bail-y in candidate assembly) - rust-lang#132597 (btree: don't leak value if destructor of key panics) - rust-lang#132911 (Pretty print async fn sugar in opaques and trait bounds) - rust-lang#133159 (Don't allow `-Zunstable-options` to take a value ) - rust-lang#133247 (Reduce integer `Display` implementation size) - rust-lang#133286 (Re-delay a resolve `bug` related to `Self`-ctor in patterns) Failed merges: - rust-lang#133215 (Fix missing submodule in `./x vendor`) r? `@ghost` `@rustbot` modify labels: rollup

…lay-impl, r=workingjubilee Reduce integer `Display` implementation size I was thinking about rust-lang#128204 and how we could reduce the size of the code and just realized that we didn't need the `_fmt` method to be implemented on signed integers, which in turns allow to simplify greatly the macro call. r? ``@workingjubilee``

…lay-impl, r=workingjubilee Reduce integer `Display` implementation size I was thinking about rust-lang#128204 and how we could reduce the size of the code and just realized that we didn't need the `_fmt` method to be implemented on signed integers, which in turns allow to simplify greatly the macro call. r? `@workingjubilee`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#127483 (Allow disabling ASan instrumentation for globals) - rust-lang#131505 (use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin) - rust-lang#132949 (Add specific diagnostic for using macro_rules macro as attribute/derive) - rust-lang#133247 (Reduce integer `Display` implementation size) - rust-lang#133286 (Re-delay a resolve `bug` related to `Self`-ctor in patterns) - rust-lang#133332 (Mark `<[T; N]>::as_mut_slice` with the `const` specifier.) - rust-lang#133366 (Remove unnecessary bool from `ExpectedFound::new`) Failed merges: - rust-lang#131859 (Update TRPL to add new Chapter 17: Async and Await) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-11-23T12:17:57Z

Failed in rollup on x86_64-gnu-llvm-18.
@bors r-

Unfortunately PR CI doesn't seem to run library tests

Log failure example

2024-11-23T12:13:40.6641277Z ---- sync::tests::test_unsized stdout ----
2024-11-23T12:13:40.6641903Z thread 'sync::tests::test_unsized' panicked at alloc/src/sync/tests.rs:406:5:
2024-11-23T12:13:40.6642447Z assertion `left == right` failed
2024-11-23T12:13:40.6642903Z   left: "[18446744073709551614, 18446744073709551613, 18446744073709551612]"
2024-11-23T12:13:40.6643366Z  right: "[1, 2, 3]"
2024-11-23T12:13:40.6643644Z stack backtrace:
2024-11-23T12:13:40.6643916Z    0: rust_begin_unwind
2024-11-23T12:13:40.6644226Z    1: core::panicking::panic_fmt
2024-11-23T12:13:40.6644584Z    2: core::panicking::assert_failed_inner
2024-11-23T12:13:40.6644976Z    3: core::panicking::assert_failed
2024-11-23T12:13:40.6645371Z    4: core::ops::function::FnOnce::call_once
2024-11-23T12:13:40.6645969Z note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
2024-11-23T12:13:40.6646428Z 
2024-11-23T12:13:40.6646434Z 
2024-11-23T12:13:40.6646539Z failures:
2024-11-23T12:13:40.6646845Z     collections::btree::set::tests::test_show
2024-11-23T12:13:40.6647275Z     collections::linked_list::tests::test_show
2024-11-23T12:13:40.6647676Z     rc::tests::test_from_box_trait
2024-11-23T12:13:40.6648046Z     rc::tests::test_into_from_raw_unsized
2024-11-23T12:13:40.6648458Z     rc::tests::test_into_from_weak_raw_unsized
2024-11-23T12:13:40.6648831Z     rc::tests::test_show
2024-11-23T12:13:40.6649135Z     sync::tests::show_arc
2024-11-23T12:13:40.6649455Z     sync::tests::test_from_box_trait
2024-11-23T12:13:40.6649832Z     sync::tests::test_into_from_raw_unsized
2024-11-23T12:13:40.6650248Z     sync::tests::test_into_from_weak_raw_unsized
2024-11-23T12:13:40.6650649Z     sync::tests::test_unsized

jieyouxu · 2024-11-23T12:22:56Z

@bors rollup=iffy

GuillaumeGomez · 2024-11-23T13:15:20Z

I have no clue what's going on in this case... On 64 linux bits, I don't get this failure and I also don't understand how this failure can happen...

GuillaumeGomez · 2024-11-23T13:17:09Z

library/core/src/fmt/num.rs

+                }
+                #[cfg(feature = "optimize_for_size")]
+                {
+                    return $gen_name((!self.unsigned_abs().$conv_fn()), *self >= 0, f);


Wait, I think the ! shouldn't be there.

jieyouxu · 2024-11-23T13:21:58Z

@GuillaumeGomez the trick about that CI job is here... locally I don't think one would ever write this, so yeah...

rust/src/ci/docker/scripts/x86_64-gnu-llvm.sh

Lines 37 to 39 in 6e1c115

    
           # Rebuild the stdlib with the size optimizations enabled and run tests again. 
        
           RUSTFLAGS_NOT_BOOTSTRAP="--cfg feature=\"optimize_for_size\"" ../x.py --stage 1 test \ 
        
               library/std library/alloc library/core

It's not that std tests don't run in PR CI (they do), however here you have code guarded by #[cfg(feature = "optimize_for_size")] which is only tested in full merge because...

rust/src/ci/docker/scripts/x86_64-gnu-llvm.sh

Lines 21 to 22 in 6e1c115

    
           # Only run the stage 1 tests on merges, not on PR CI jobs. 
        
           if [[ -z "${PR_CI_JOB}" ]]; then

jieyouxu · 2024-11-23T13:32:19Z

Actually this PR intentionally tries to adjust code size, so
@bors rollup=never

GuillaumeGomez · 2024-11-23T13:33:36Z

The problem was indeed because I never tested with the optimize_for_size feature. :)

GuillaumeGomez · 2024-11-24T21:41:54Z

Forgot to re-r+ it...

@bors r=workingjubilee

bors · 2024-11-24T21:41:56Z

📌 Commit 0d4b52f has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2024-11-25T11:03:44Z

⌛ Testing commit 0d4b52f with merge 7db7489...

bors · 2024-11-25T13:47:22Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 7db7489 to master...

rust-timer · 2024-11-25T15:05:11Z

Finished benchmarking commit (7db7489): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (secondary 0.1%)

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

Cycles

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

Binary size

Results (primary -0.0%, secondary -0.1%)

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.0%	[0.0%, 0.1%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	8
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	36
All ❌✅ (primary)	-0.0%	[-0.1%, 0.1%]	12

Bootstrap: 797.447s -> 796.592s (-0.11%)
Artifact size: 336.31 MiB -> 336.27 MiB (-0.01%)

Reduce integer Display implementation size

d318878

rustbot assigned workingjubilee Nov 20, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 20, 2024

workingjubilee reviewed Nov 20, 2024

View reviewed changes

library/core/src/fmt/num.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the reduce-integer-display-impl branch from 75f1ff7 to a991f25 Compare November 20, 2024 22:51

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 Nov 20, 2024

matthiaskrgr mentioned this pull request Nov 21, 2024

Rollup of 6 pull requests #133273

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 21, 2024

GuillaumeGomez force-pushed the reduce-integer-display-impl branch 2 times, most recently from 81d470f to ab38c6d Compare November 21, 2024 11:35

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2024

GuillaumeGomez mentioned this pull request Nov 21, 2024

Rollup of 8 pull requests #133296

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 22, 2024

workingjubilee reviewed Nov 22, 2024

View reviewed changes

compiler-errors mentioned this pull request Nov 22, 2024

Rollup of 8 pull requests #133350

Closed

compiler-errors mentioned this pull request Nov 23, 2024

Rollup of 6 pull requests #133359

Closed

matthiaskrgr mentioned this pull request Nov 23, 2024

Rollup of 7 pull requests #133375

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 23, 2024

GuillaumeGomez commented Nov 23, 2024

View reviewed changes

Improve code by using unsigned_abs

0d4b52f

GuillaumeGomez force-pushed the reduce-integer-display-impl branch from 30c7df6 to 0d4b52f Compare November 23, 2024 13:17

lukas-code mentioned this pull request Nov 23, 2024

btree: don't leak value if destructor of key panics #132597

Merged

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 25, 2024

bors merged commit 7db7489 into rust-lang:master Nov 25, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Nov 25, 2024

GuillaumeGomez deleted the reduce-integer-display-impl branch November 25, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce integer `Display` implementation size #133247

Reduce integer `Display` implementation size #133247

GuillaumeGomez commented Nov 20, 2024

workingjubilee commented Nov 20, 2024

GuillaumeGomez commented Nov 20, 2024

bors commented Nov 20, 2024

matthiaskrgr commented Nov 21, 2024

GuillaumeGomez commented Nov 21, 2024

bors commented Nov 21, 2024

workingjubilee commented Nov 22, 2024

workingjubilee commented Nov 22, 2024

workingjubilee Nov 22, 2024 •

edited

Loading

GuillaumeGomez Nov 22, 2024

workingjubilee Nov 22, 2024

jieyouxu commented Nov 23, 2024 •

edited

Loading

jieyouxu commented Nov 23, 2024

GuillaumeGomez commented Nov 23, 2024 •

edited

Loading

GuillaumeGomez Nov 23, 2024

workingjubilee Nov 25, 2024

jieyouxu commented Nov 23, 2024 •

edited

Loading

jieyouxu commented Nov 23, 2024 •

edited

Loading

GuillaumeGomez commented Nov 23, 2024

GuillaumeGomez commented Nov 24, 2024

bors commented Nov 24, 2024

bors commented Nov 25, 2024

bors commented Nov 25, 2024

rust-timer commented Nov 25, 2024

Reduce integer Display implementation size #133247

Reduce integer Display implementation size #133247

Conversation

GuillaumeGomez commented Nov 20, 2024

workingjubilee commented Nov 20, 2024

GuillaumeGomez commented Nov 20, 2024

bors commented Nov 20, 2024

matthiaskrgr commented Nov 21, 2024

GuillaumeGomez commented Nov 21, 2024

bors commented Nov 21, 2024

workingjubilee commented Nov 22, 2024

workingjubilee commented Nov 22, 2024

workingjubilee Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Nov 23, 2024 • edited Loading

jieyouxu commented Nov 23, 2024

GuillaumeGomez commented Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Nov 23, 2024 • edited Loading

jieyouxu commented Nov 23, 2024 • edited Loading

GuillaumeGomez commented Nov 23, 2024

GuillaumeGomez commented Nov 24, 2024

bors commented Nov 24, 2024

bors commented Nov 25, 2024

bors commented Nov 25, 2024

rust-timer commented Nov 25, 2024

Overall result: ✅ improvements - no action needed

Reduce integer `Display` implementation size #133247

Reduce integer `Display` implementation size #133247

workingjubilee Nov 22, 2024 •

edited

Loading

jieyouxu commented Nov 23, 2024 •

edited

Loading

GuillaumeGomez commented Nov 23, 2024 •

edited

Loading

jieyouxu commented Nov 23, 2024 •

edited

Loading

jieyouxu commented Nov 23, 2024 •

edited

Loading