Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster parsing for lower numbers for radix up to 16 #83371

Closed
wants to merge 8 commits into from

Conversation

gilescope
Copy link
Contributor

While musing on #83088 I noticed that we could use str len to establish if the input was guarenteed not to overflow the containing type.

From what I can see this is conservative enough (i64 having the least wiggle room) that we can safely skip the overflow checks because we have ruled out overflow and that seems to improve performance by 1/4 to 1/3 for numbers in the range. Given that most numbers are statistically on the small side rather than near the type limits, I'm tempted by this. As it stands it doesn't really help i8 much as it's so small a range of numbers, but it does work well with u8 ( FF ) which is far more common.

I don't like adding unsafety, but this might be worth it.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2021
@gilescope gilescope changed the title WIP: Significantly faster parsing for lower numbers for radix up to 16 WIP: Faster parsing for lower numbers for radix up to 16 Mar 22, 2021
@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 22, 2021
library/core/src/num/mod.rs Outdated Show resolved Hide resolved
library/core/src/num/mod.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@crlf0710
Copy link
Member

@gilescope Ping from triage! CI is still red here. Mind fixing that?

@crlf0710 crlf0710 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 23, 2021
@pickfire
Copy link
Contributor

I believe the other pull request superseeds this pull request. Which @gilescope is still experimenting with.

@gilescope
Copy link
Contributor Author

@pickfire while I'd like to find a way to improve base10 parsing specifically, this is a simple speedup for all radix with little down side. It focuses on small numbers which are in general more common so it's likely to kick in most of the time.

I think given the simplicity we should move forward with this PR.

@gilescope gilescope changed the title WIP: Faster parsing for lower numbers for radix up to 16 Faster parsing for lower numbers for radix up to 16 May 3, 2021
@pickfire
Copy link
Contributor

pickfire commented May 3, 2021

small numbers

You may also want to write down how small there.

library/core/src/num/mod.rs Outdated Show resolved Hide resolved
library/core/src/num/mod.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

library/core/src/num/mod.rs Show resolved Hide resolved
library/core/src/num/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@LingMan LingMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more nits and you might want to squash down your commits a bit but otherwise LGTM.

library/core/src/num/mod.rs Outdated Show resolved Hide resolved
library/core/src/num/mod.rs Show resolved Hide resolved
@gilescope
Copy link
Contributor Author

Hmm, redoing the perf testing pulling out let unchecked_additive_op = to prevent the duplication does lead to a perormance penalty. :-(

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021
@crlf0710
Copy link
Member

@gilescope Ping from triage, what's next steps here?

@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2021
@crlf0710
Copy link
Member

@gilescope Ping from triage, any updates here?

@camelid camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 2, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 20, 2021
@camelid camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2021
@JohnCSimon
Copy link
Member

@gilescope Ping from triage. Can you please post the status of this MR?

@LingMan
Copy link
Contributor

LingMan commented Nov 18, 2021

@gilescope Does replacing the contents of the unsafe block with this, resolve the performance issue you saw?

            macro_rules! run_loop {
                ($unchecked_additive_op:ident) => {
                    for &c in digits {
                        result = result.unchecked_mul(radix);
                        let x = (c as char).to_digit(radix).ok_or(PIE { kind: InvalidDigit })?;
                        result = T::$unchecked_additive_op(&result, x);
                    }
                }
            }

            if is_positive { run_loop!(unchecked_add) } else { run_loop!(unchecked_sub) };

@JohnCSimon
Copy link
Member

@gilescope
Ping from triage: I'm closing this one due to not being touched since june , please feel to reopen when you are ready to continue with this.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Dec 12, 2021
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Dec 12, 2021
@gilescope
Copy link
Contributor Author

@JohnCSimon can we reopen this PR. I didn't see LingMan's comment. I think this is good now with LingMan's change. Is it possible to kick off a perf run? (I've rebased to bring this up to date with master)

@gilescope
Copy link
Contributor Author

(Sorry I've been away a long time.)

@kennytm
Copy link
Member

kennytm commented Mar 28, 2022

the branch was force-pushed or recreated so the PR can't be re-opened. you could submit a new PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2022
Faster parsing for lower numbers for radix up to 16 (cont.)

( Continuation of rust-lang#83371 )

With LingMan's change I think this is potentially ready.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
Faster parsing for lower numbers for radix up to 16 (cont.)

( Continuation of rust-lang/rust#83371 )

With LingMan's change I think this is potentially ready.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.