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

replace hand-rolled lexicalCast #4473

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

dangell7
Copy link
Collaborator

High Level Overview of Change

Replace hand-rolled code with std::from_chars for better performance and maintainability. The only minor change in semantics is the inability to parse the + sign, but that functionality is unused in practice.

Context of Change

Type of Change

  • Refactor (non-breaking change that only restructures code)

@intelliot
Copy link
Collaborator

better performance

Can you quantify the change in performance in any way?

@dangell7
Copy link
Collaborator Author

better performance

Can you quantify the change in performance in any way?

This change does not have a directly measurable impact in performance and synthetic benchmarks are unlikely to be helpful. The C++ std::from_chars is intended to be as efficient and fast as possible and it's unlikely that it will be any slower than the code that it replaces. And given that it also reduces the amount of hand-rolled code it's a net gain.

Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

PR looks fine to me but I'm reluctant to remove support for parsing + sign.

@intelliot
Copy link
Collaborator

I'm reluctant to remove support for parsing + sign.

@gregtatcam why?

@gregtatcam
Copy link
Collaborator

I'm reluctant to remove support for parsing + sign.

@gregtatcam why?

I agree that it's probably not used in practice. But are we 100% sure that it's never used? And it's easy to add support for + parsing.

@intelliot
Copy link
Collaborator

are we 100% sure that it's never used? And it's easy to add support for + parsing.

@dangell7 thoughts?

@dangell7
Copy link
Collaborator Author

dangell7 commented Apr 3, 2023

I'm not sure of any instance where someone would use a "+" in an integer string. I had thought of e+10 but technically thats not an integer. I can test this though to make sure. I'm also thinking about the error it could cause and that might have negative effects.

If we need to parse "+" we can always revert it.

No rush let me check these 2 tests to make sure theres not a nefarious way to use this.

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

I like that this replaces some of our code with std::code.

On the '+' issue, I recommend just checking in[0] == '+' && isdigit(in[1]) prior to calling from_chars, being careful about strings of length 0 and 1 of course. It likely won't be used, but it is cheap to support this existing outward facing API.

@intelliot
Copy link
Collaborator

@dangell7 can you address the '+' issue as discussed above?

@@ -84,6 +84,9 @@ struct LexicalCast<Out, std::string>
auto first = in.data();
auto last = in.data() + in.size();

if (first != last && *first == '+')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have to check the next one is digit. See @HowardHinnant comment.

Copy link
Contributor

@nbougalis nbougalis May 26, 2023

Choose a reason for hiding this comment

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

I don't think so - the semantics seem correct to me. If it's a plus and the next character is not a digit, then std::from_chars will return some kind of failure and the check, which ensures that (a) not failure is returned; and (b) that the whole string was parsed and nothing was left over will work correctly.

Check out this example. As you can see, the only cases where the semantics don't match is when you have a numeric value prefixed with a + sign. The "new" code does what we want, and the old one just fails.

@intelliot intelliot added this to the 1.12 milestone May 28, 2023
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Optional drive-by fix: #include <iostream> is unnecessary and adds a small amount of code bloat. Needs removing from LexicalCast.h.

@dangell7
Copy link
Collaborator Author

dangell7 commented Jun 1, 2023

Optional drive-by fix: #include <iostream> is unnecessary and adds a small amount of code bloat. Needs removing from LexicalCast.h.

Thank you for this. Building and testing now. Is there a program you're using to identify these or any recommendations you can make?

@intelliot intelliot added Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Tech Debt Non-urgent improvements labels Jun 23, 2023
@intelliot intelliot merged commit 534a365 into XRPLF:develop Jun 27, 2023
@intelliot
Copy link
Collaborator

Is there a program you're using to identify these or any recommendations you can make?

No program. <iostream> is the only header in the std::lib that can cause a slight run-time cost just for including it. So it's special and Howard tends to look for it.

ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Jul 10, 2023
Replace hand-rolled code with std::from_chars for better
maintainability.

The C++ std::from_chars function is intended to be as fast as possible,
so it is unlikely to be slower than the code it replaces. This change is
a net gain because it reduces the amount of hand-rolled code.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
Replace hand-rolled code with std::from_chars for better
maintainability.

The C++ std::from_chars function is intended to be as fast as possible,
so it is unlikely to be slower than the code it replaces. This change is
a net gain because it reduces the amount of hand-rolled code.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
Replace hand-rolled code with std::from_chars for better
maintainability.

The C++ std::from_chars function is intended to be as fast as possible,
so it is unlikely to be slower than the code it replaces. This change is
a net gain because it reduces the amount of hand-rolled code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Tech Debt Non-urgent improvements
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants