perf: improve write_fmt to handle simple strings

[nyurik]

In case format string has no arguments, simplify its implementation with a direct call to output.write_str(value). This builds on @dtolnay original suggestion. This does not change any expectations because the original fn write() implementation calls write_str for parts of the format string.

write!(f, "text")  ->  f.write_str("text")

 /// [`write!`]: crate::write!
+#[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn write(output: &mut dyn Write, args: Arguments<'_>) -> Result {
+    if let Some(s) = args.as_str() { output.write_str(s) } else { write_internal(output, args) }
+}
+
+/// Actual implementation of the [`write`], but without the simple string optimization.
+fn write_internal(output: &mut dyn Write, args: Arguments<'_>) -> Result {
     let mut formatter = Formatter::new(output);
     let mut idx = 0;

• Hopefully it will improve the simple case for the Tracking issue for improving std::fmt::Arguments and format_args!() #99012
• Another related (original?) issues write!(wr,"foo") is 10% to 72% slower than wr.write("foo".as_bytes()) #10761
• Previous similar attempt to fix it by by @Kobzol Leverage fmt::Arguments::as_str in the write! macro #100700

CC: @m-ou-se as probably the biggest expert in everything format!

[rustbot]

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Nilstrieb]

Can you run some benchmarks for our original motivating program to see whether this helps?
To ensure a good comparison target I'm gonna do a try build
@bors try
I think rustdoc still uses write in many places, so a perf run might reveal something as well.
@rust-timer queue

[Nilstrieb]

@bors try

[bors]

⌛ Trying commit 75808d1 with merge 09bfbad...

[nyurik]

I tried this code, running it with the latest main branch and compiling using RUSTFLAGS="--emit asm" cargo +stage2 build --release . Note that in the second case it seems to have done the tail call elimination!

#[no_mangle]
#[inline(never)]
pub fn fmt_abcd(f: &mut Formatter<'_>) -> fmt::Result {
    write!(f, "ABCXYZ")
}

Before the change:

fmt_abcd:
	.cfi_startproc
	subq	$56, %rsp
	.cfi_def_cfa_offset 64
	leaq	.L__unnamed_2(%rip), %rax
	movq	%rax, 8(%rsp)
	movq	$1, 16(%rsp)
	leaq	.L__unnamed_3(%rip), %rax
	movq	%rax, 24(%rsp)
	xorps	%xmm0, %xmm0
	movups	%xmm0, 32(%rsp)
	leaq	8(%rsp), %rsi
	callq	*_ZN4core3fmt9Formatter9write_fmt17hf6272f62fa2a64dfE@GOTPCREL(%rip)
	addq	$56, %rsp
	.cfi_def_cfa_offset 8
	retq
.Lfunc_end5:

After the change:

fmt_abcd:
	.cfi_startproc
	movq	32(%rdi), %rax
	movq	40(%rdi), %rcx
	movq	24(%rcx), %rcx
	leaq	.L__unnamed_2(%rip), %rsi
	movl	$6, %edx
	movq	%rax, %rdi
	jmpq	*%rcx
.Lfunc_end5:

[Kobzol]

In the past, this didn't really help (#100700), but we didn't have runtime benchmarks then.

[bors]

☀️ Try build successful - checks-actions
Build commit: 09bfbad (09bfbad0db2dd7aa4e4c6c7e5dbb8a410a72fa4f)

[rust-timer]

Finished benchmarking commit (09bfbad): comparison URL.

Overall result: ✅ improvements - no 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.

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

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.1%	[0.2%, 2.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.9%	[-13.4%, -2.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.4%	[-13.4%, 2.1%]	4

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

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.2%]	40
Regressions ❌ (secondary)	0.7%	[0.4%, 1.0%]	6
Improvements ✅ (primary)	-0.3%	[-1.1%, -0.0%]	18
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	4
All ❌✅ (primary)	-0.0%	[-1.1%, 0.2%]	58

Bootstrap: 662.665s -> 665.299s (0.40%)
Artifact size: 308.47 MiB -> 308.48 MiB (0.00%)

[Kobzol]

This looks a bit worrisome, but it could also be noise, we don't yet have history charts for runtime benchmarks.

[nyurik]

@Kobzol thx, I tried to dig through the graphs - but there is so much data, I am not certain what to look at. The worst test I see is branch misses - it went up by 23773.11% (!!) for fmt-write-str. The rest of the results seem relatively consistent and benign - but again, I might be looking at the wrong thing.

What are the next steps for this? Are there any reliable way to figure out what's going on with a certain test? Note that micro benchmarks show nearly 80% perf gain when using write_str, and my assembly experiment above shows a significant drop in the instruction count - which looks identical to what a write_str would produce.

[Kobzol]

I would take the runtime benchmarks results with a grain of salt. That being said, a lot of branch misses makes sense. The call is resolved with dynamic dispatch, through a v-table, and this condition might be very badly predicted, because it will be true for some values and false for others, quite randomly.

[nyurik]

my understanding was that this new if/else gets compiled away because the .as_str() is a const fn that gets inlined, plus the value itself is known at the compile time - so the compiler should fully remove one of the branches - ending up with either the original code (for complex formats), or the new shorter branch for the simple str ... or am i reasoning incorrectly?

[Kobzol]

Sorry, my point about the v-table was wrong, there's no dynamic dispatch, the function is implemented on Arguments directly, I didn't notice that. The fmt calls on Arguments::args are resolved via dynamic dispatch.

On the other hand, the as_str call might not be fully resolved in compile time, for that the compiler would have to inline everything, and I'm not sure if that happens currently. It would be interesting to check if that inlining happens in --release.

[cuviper]

I wonder if we can use is_val_statically_known for this?

[rust-timer]

Finished benchmarking commit (dc99c35): comparison URL.

Overall result: ❌ regressions - no 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.

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

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)	5.4%	[1.0%, 8.7%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	5.4%	[1.0%, 8.7%]	5

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.9%]	27
Regressions ❌ (secondary)	0.1%	[0.0%, 0.6%]	17
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.2%, 0.9%]	34

Bootstrap: 640.589s -> 639.335s (-0.20%)
Artifact size: 308.63 MiB -> 308.68 MiB (0.02%)

[nyurik]

Are these good results or not? Or are we not even doing relevant perf testing for the simple write! implementation? What are the next steps?

[Kobzol]

I don't think that our current benchmarks in rustc-perf are a good fit for a change like this. Perhaps a micro-benchmark in stdlib would be better. That being said, even a microbenchmark won't tell us the effects on non-trivial programs. That's why it's challenging to do changes like this.

[nyurik]

@Kobzol are the assembly output changes a good indicator? See my comment above #121001 (comment)

[Kobzol]

It's definitely a way of evaluating the potential perf. effects of this change :) I'm not a libs reviewer, so I'll leave the evaluation of this change to others.

[cuviper]

@bors r+

[bors]

📌 Commit c85a9df has been approved by cuviper

It is now in the queue for this repository.

[bors]

⌛ Testing commit c85a9df with merge 5a1e544...

[bors]

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 5a1e544 to master...

[rust-timer]

Finished benchmarking commit (5a1e544): 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)	1.7%	[0.5%, 3.6%]	4
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	1.7%	[0.5%, 3.6%]	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.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.4%	[-6.5%, -4.2%]	2
Improvements ✅ (secondary)	-3.2%	[-6.2%, -2.1%]	32
All ❌✅ (primary)	-2.8%	[-6.5%, 2.3%]	3

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)	2.2%	[1.1%, 3.7%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[1.1%, 3.7%]	3

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%, 1.8%]	34
Regressions ❌ (secondary)	0.2%	[0.0%, 1.0%]	19
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.1%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.2%, 1.8%]	39

Bootstrap: 645.309s -> 645.372s (0.01%)
Artifact size: 174.97 MiB -> 175.01 MiB (0.02%)

[Mark-Simulacrum]

Regressions look real, but are perhaps expected -- LLVM can optimize more with this change, so it'll spend more time doing that. I'm going to mark as triaged.

[nnethercote]

Are the binary size regressions expected?

[nyurik]

@nnethercote thx for the ping, I had another go at in-depth assembly digging - it seems the inlined fn with dyn keyword generates considerable extra code for each call when using for formatting (vs non-formatting) case. So I created #122059 that goes back to the as_const_str approach - not as clean code-wise, but generates better results (and faster when used with formatted string)

[nnethercote]

@nyurik: thanks for looking into it!