Improve the floating point parser in dec2flt. #107007

TDecking · 2023-01-17T21:23:07Z

Greetings everyone,

I've benn studying the rust floating point parser recently and made the following tweaks:

Remove all remaining traces of unsafe. The parser is now 100% safe Rust.
The trick in which eight digits are processed in parallel is now in a loop.
Parsing of inf/NaN values has been reworked.

On my system, the changes result in performance improvements for some input values.

rustbot · 2023-01-17T21:23:15Z

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

Please see the contribution instructions for more information.

rustbot · 2023-01-17T23:15:50Z

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

scottmcm · 2023-01-18T07:36:15Z

library/core/src/num/dec2flt/common.rs

+        let e = self[4];
+        let f = self[5];
+        let g = self[6];
+        let h = self[7];


suggestion: maybe use one of the unstable methods to get an array without needing to write this all out?

Maybe something like

let first_8 = self.split_array_ref().0; u64::from_le_bytes(*first_8)

Well, I've already written it all out. The suggested code does produce the desired assembly while being a bit shorter,
but I otherwise don't see the need to change anything. How likely is stabilisation of this method?

Something like what @scottmcm suggests seems more readable here. You can also do something like u64::from_le_bytes(self[..8].try_into().unwrap()) which probably works equally well and should be stable. (But we should confirm against generated code).

TDecking · 2023-03-30T21:46:55Z

r? t-libs

rustbot · 2023-03-30T21:46:58Z

Failed to set assignee to t-libs: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

TDecking · 2023-03-30T23:01:11Z

r? libs

library/core/src/num/dec2flt/common.rs

library/core/src/num/dec2flt/decimal.rs

TDecking · 2023-04-03T15:31:48Z

@Mark-Simulacrum I've made some changes corresponding to your comments.

read_u64 and write_u64 now use copy_from_slice.
reinserted the parse_digits function by adding it to the ByteSlice trait.

I've analyzed the changes using godbolt. read_u64 and write_u64 are reduced to one memory access and one bound check.
The latter should be elided after inlining. parse_digits relies on pattern matching on the slice instead of using split_first,
as #109328 makes it neccessary in order to avoid possible regressions.

TDecking · 2023-04-07T10:07:55Z

@rustbot ready

Mark-Simulacrum · 2023-04-09T15:21:40Z

Please squash away the fixup commits, r=me with that done.

klensy · 2023-04-09T16:46:29Z

https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy 69c788d

* Remove all remaining traces of unsafe. * Put `parse_8digits` inside a loop. * Rework parsing of inf/NaN values.

TDecking · 2023-04-10T11:53:42Z

@Mark-Simulacrum Commits are squashed, and the merge commit has been removed to comply with the No-Merge policy.

Mark-Simulacrum · 2023-04-10T12:28:46Z

@bors r+ rollup=never

bors · 2023-04-10T12:28:48Z

📌 Commit 0f96c71 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-04-10T14:09:12Z

⌛ Testing commit 0f96c71 with merge a732883...

bors · 2023-04-10T16:41:16Z

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

rust-timer · 2023-04-10T17:59:03Z

Finished benchmarking commit (a732883): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -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)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-0.7%	[-1.5%, -0.4%]	9
Improvements ✅ (secondary)	-1.3%	[-1.3%, -1.3%]	1
All ❌✅ (primary)	-0.6%	[-1.5%, 1.2%]	10

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

rustbot assigned joshtriplett Jan 17, 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 Jan 17, 2023

TDecking marked this pull request as draft January 17, 2023 21:33

TDecking marked this pull request as ready for review January 17, 2023 23:15

scottmcm reviewed Jan 18, 2023

View reviewed changes

TDecking marked this pull request as draft March 21, 2023 23:47

This comment has been minimized.

Sign in to view

TDecking marked this pull request as ready for review March 23, 2023 19:43

rustbot assigned Mark-Simulacrum and unassigned joshtriplett Mar 30, 2023

Mark-Simulacrum reviewed Apr 2, 2023

View reviewed changes

library/core/src/num/dec2flt/common.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Apr 2, 2023

View reviewed changes

library/core/src/num/dec2flt/decimal.rs Show resolved Hide resolved

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2023

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2023

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2023

TDecking force-pushed the float_parsing_improvments branch from afce93c to 4bded37 Compare April 9, 2023 15:52

Improve the floating point parser in dec2flt.

0f96c71

* Remove all remaining traces of unsafe. * Put `parse_8digits` inside a loop. * Rework parsing of inf/NaN values.

TDecking force-pushed the float_parsing_improvments branch from 4bded37 to 0f96c71 Compare April 9, 2023 22:52

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

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

bors merged commit a732883 into rust-lang:master Apr 10, 2023

rustbot added this to the 1.70.0 milestone Apr 10, 2023

TDecking deleted the float_parsing_improvments branch April 10, 2023 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the floating point parser in dec2flt. #107007

Improve the floating point parser in dec2flt. #107007

TDecking commented Jan 17, 2023 •

edited

Loading

rustbot commented Jan 17, 2023

rustbot commented Jan 17, 2023

scottmcm Jan 18, 2023

TDecking Jan 19, 2023

Mark-Simulacrum Apr 2, 2023

This comment has been minimized.

This comment has been minimized.

TDecking commented Mar 30, 2023

rustbot commented Mar 30, 2023

TDecking commented Mar 30, 2023

This comment has been minimized.

TDecking commented Apr 3, 2023

TDecking commented Apr 7, 2023

Mark-Simulacrum commented Apr 9, 2023

klensy commented Apr 9, 2023

TDecking commented Apr 10, 2023

Mark-Simulacrum commented Apr 10, 2023

bors commented Apr 10, 2023

bors commented Apr 10, 2023

bors commented Apr 10, 2023

rust-timer commented Apr 10, 2023

Improve the floating point parser in dec2flt. #107007

Improve the floating point parser in dec2flt. #107007

Conversation

TDecking commented Jan 17, 2023 • edited Loading

rustbot commented Jan 17, 2023

rustbot commented Jan 17, 2023

scottmcm Jan 18, 2023

Choose a reason for hiding this comment

TDecking Jan 19, 2023

Choose a reason for hiding this comment

Mark-Simulacrum Apr 2, 2023

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

TDecking commented Mar 30, 2023

rustbot commented Mar 30, 2023

TDecking commented Mar 30, 2023

This comment has been minimized.

TDecking commented Apr 3, 2023

TDecking commented Apr 7, 2023

Mark-Simulacrum commented Apr 9, 2023

klensy commented Apr 9, 2023

TDecking commented Apr 10, 2023

Mark-Simulacrum commented Apr 10, 2023

bors commented Apr 10, 2023

bors commented Apr 10, 2023

bors commented Apr 10, 2023

rust-timer commented Apr 10, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

TDecking commented Jan 17, 2023 •

edited

Loading