Add shortcut for Grisu3 algorithm. #110389

mazong1123 · 2023-04-16T05:50:58Z

While Grisu3 is way more faster for most numbers compare to Dragon4, the fall back to Dragon4 procedure for certain numbers could cause some performance regressions compare to use Dragon4 directly. Mitigating the regression caused by falling back is important for a largely used core library.

In Grisu3 algorithm implementation, there's a shortcut to jump out earlier when the fractional or integrals cannot meet the requirement of requested digits. This could significantly improve the performance of converting floating number to string as it falls back even without starting trying the algorithm.

The original idea is from the .NET implementation and the code was originally added in this PR. This shortcut has been shipped long time ago and has been proved working.

Fix #110129

rustbot · 2023-04-16T05:51:05Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2023-04-16T05:51:08Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

mazong1123 · 2023-04-16T05:54:41Z

@bors try @rust-timer queue

bors · 2023-04-16T05:54:44Z

@mazong1123: 🔑 Insufficient privileges: not in try users

mazong1123 · 2023-04-16T05:56:30Z

@simulacrum looks like I don't have permission to request a perf benchmark. As this change is perf related, is there any chance to grant me the right permission?

mazong1123 · 2023-04-16T06:05:04Z

Here is the benchmark data for .NET with the shortcut. Doing similar benchmark tests in rust would be great.

fee1-dead · 2023-04-16T06:28:10Z

@bors try @rust-timer queue

bors · 2023-04-16T06:28:18Z

⌛ Trying commit 9968365655939cad46b5ec70ff3f1d201d150ba8 with merge 09a6ce7b9a3eb5f779b7015c5138e9b8c54ff00c...

bors · 2023-04-16T08:11:01Z

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

rust-timer · 2023-04-16T11:47:27Z

Finished benchmarking commit (09a6ce7b9a3eb5f779b7015c5138e9b8c54ff00c): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-3.7%	[-3.7%, -3.7%]	1
Improvements ✅ (secondary)	-3.9%	[-5.2%, -2.4%]	3
All ❌✅ (primary)	-3.7%	[-3.7%, -3.7%]	1

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)	4.7%	[4.3%, 5.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

fee1-dead · 2023-04-16T12:09:39Z

This can be expected since rustc perf is mostly related to the performance of the compiler. You should consider writing your own benchmarks to see the performance changes.

mazong1123 · 2023-04-16T19:35:16Z

Appears current rust benchmark for Grisu3 does not have numbers to cover the fallback cases. I've added 3 benchmarks: bench_one_exact_inf, bench_trailing_zero_exact_inf and bench_halfway_point_exact_inf. Below are the benchmark results in my local machine:

Before change

benchmarks:
num::flt2dec::strategy::grisu::bench_big_exact_12 75.00ns/iter +/- 4.00ns
num::flt2dec::strategy::grisu::bench_big_exact_3 35.00ns/iter +/- 1.00ns
num::flt2dec::strategy::grisu::bench_big_exact_inf 33.15µs/iter +/- 5.16µs
num::flt2dec::strategy::grisu::bench_big_shortest 93.00ns/iter +/- 5.00ns
num::flt2dec::strategy::grisu::bench_halfway_point_exact_inf 475.00ns/iter +/- 25.00ns
num::flt2dec::strategy::grisu::bench_one_exact_inf 476.00ns/iter +/- 20.00ns
num::flt2dec::strategy::grisu::bench_small_exact_12 58.00ns/iter +/- 3.00ns
num::flt2dec::strategy::grisu::bench_small_exact_3 32.00ns/iter +/- 1.00ns
num::flt2dec::strategy::grisu::bench_small_exact_inf 1.00µs/iter +/- 27.00ns
num::flt2dec::strategy::grisu::bench_small_shortest 51.00ns/iter +/- 2.00ns
num::flt2dec::strategy::grisu::bench_trailing_zero_exact_inf 490.00ns/iter +/- 16.00ns

After change

benchmarks:
num::flt2dec::strategy::grisu::bench_big_exact_12 77.00ns/iter +/- 3.00ns
num::flt2dec::strategy::grisu::bench_big_exact_3 35.00ns/iter +/- 1.00ns
num::flt2dec::strategy::grisu::bench_big_exact_inf 33.11µs/iter +/- 2.61µs
num::flt2dec::strategy::grisu::bench_big_shortest 92.00ns/iter +/- 7.00ns
num::flt2dec::strategy::grisu::bench_halfway_point_exact_inf 413.00ns/iter +/- 31.00ns
num::flt2dec::strategy::grisu::bench_one_exact_inf 416.00ns/iter +/- 65.00ns
num::flt2dec::strategy::grisu::bench_small_exact_12 62.00ns/iter +/- 3.00ns
num::flt2dec::strategy::grisu::bench_small_exact_3 32.00ns/iter +/- 1.00ns
num::flt2dec::strategy::grisu::bench_small_exact_inf 985.00ns/iter +/- 319.00ns
num::flt2dec::strategy::grisu::bench_small_shortest 54.00ns/iter +/- 8.00ns
num::flt2dec::strategy::grisu::bench_trailing_zero_exact_inf 434.00ns/iter +/- 26.00ns

Benchmark	Before	After	Improvement
(num::flt2dec::strategy::grisu::bench_halfway_point_exact_inf)	475.00ns/iter	413.00ns/iter	13.05%
(num::flt2dec::strategy::grisu::bench_one_exact_inf)	476.00ns/iter	416.00ns/iter	12.60%
num::flt2dec::strategy::grisu::bench_trailing_zero_exact_inf	490.00ns/iter	434.00ns/iter	11.42%

mazong1123 · 2023-04-17T21:30:23Z

Bumping. Anyone has a chance to review?

mazong1123 · 2023-04-17T21:31:28Z

@rustbot review

mazong1123 · 2023-04-22T03:03:27Z

@Mark-Simulacrum if you have time :)

Mark-Simulacrum · 2023-04-22T23:02:00Z

Please be aware the reviews can take 7-14 days; the reviewer pool is predominantly composed of volunteers. I will get to this fairly soon, likely today or tomorrow, but rest assured that as long as it's in S-waiting-on-review it's in my queue.

library/core/src/num/flt2dec/strategy/grisu.rs

Mark-Simulacrum · 2023-04-24T12:39:07Z

r=me with nit fixed

Check requested digit length and the fractional or integral parts of the number. Falls back earlier without trying the Grisu algorithm if the specific condition meets. Fix rust-lang#110129

mazong1123 · 2023-04-25T04:39:48Z

Thanks @Mark-Simulacrum . Updated accordingly. Commits are squashed as well.

fee1-dead · 2023-04-25T05:17:56Z

@bors r=Mark-Simulacrum

bors · 2023-04-25T05:17:58Z

📌 Commit b0a85d6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-04-25T07:05:54Z

⌛ Testing commit b0a85d6 with merge 91b61a4...

bors · 2023-04-25T09:18:51Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 91b61a4 to master...

rust-timer · 2023-04-25T12:52:13Z

Finished benchmarking commit (91b61a4): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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

Cycles

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

rustbot assigned Mark-Simulacrum Apr 16, 2023

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 Apr 16, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 16, 2023

Mark-Simulacrum reviewed Apr 24, 2023

View reviewed changes

library/core/src/num/flt2dec/strategy/grisu.rs Outdated Show resolved Hide resolved

mazong1123 force-pushed the add-shortcut-for-grisu3 branch from 173069e to 705f7a5 Compare April 25, 2023 03:33

Add shortcut for Grisu3 algorithm.

b0a85d6

Check requested digit length and the fractional or integral parts of the number. Falls back earlier without trying the Grisu algorithm if the specific condition meets. Fix rust-lang#110129

mazong1123 force-pushed the add-shortcut-for-grisu3 branch from 705f7a5 to b0a85d6 Compare April 25, 2023 03: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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 25, 2023

bors merged commit 91b61a4 into rust-lang:master Apr 25, 2023

rustbot added this to the 1.71.0 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shortcut for Grisu3 algorithm. #110389

Add shortcut for Grisu3 algorithm. #110389

mazong1123 commented Apr 16, 2023 •

edited

Loading

rustbot commented Apr 16, 2023

rustbot commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

This comment has been minimized.

mazong1123 commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

fee1-dead commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

bors commented Apr 16, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

fee1-dead commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

mazong1123 commented Apr 17, 2023

mazong1123 commented Apr 17, 2023

mazong1123 commented Apr 22, 2023

Mark-Simulacrum commented Apr 22, 2023

Mark-Simulacrum commented Apr 24, 2023

mazong1123 commented Apr 25, 2023

fee1-dead commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

rust-timer commented Apr 25, 2023

Add shortcut for Grisu3 algorithm. #110389

Add shortcut for Grisu3 algorithm. #110389

Conversation

mazong1123 commented Apr 16, 2023 • edited Loading

rustbot commented Apr 16, 2023

rustbot commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

This comment has been minimized.

mazong1123 commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

fee1-dead commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

bors commented Apr 16, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

fee1-dead commented Apr 16, 2023

mazong1123 commented Apr 16, 2023

Before change

After change

mazong1123 commented Apr 17, 2023

mazong1123 commented Apr 17, 2023

mazong1123 commented Apr 22, 2023

Mark-Simulacrum commented Apr 22, 2023

Mark-Simulacrum commented Apr 24, 2023

mazong1123 commented Apr 25, 2023

fee1-dead commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

rust-timer commented Apr 25, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

mazong1123 commented Apr 16, 2023 •

edited

Loading