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

[WIP] Minimum stod change #3945

Closed
wants to merge 1 commit into from

Conversation

alexbeattie42
Copy link
Contributor

@alexbeattie42 alexbeattie42 commented Oct 18, 2024

Fixes issue #3924

Brief summary of changes

String to Decimal Conversion

The above fixed the reading/writing problems but the code was still failing. Even with the imbuing of the input and output file stream, the parser converting strings to decimals is still using the system locale. After enough digging and exploration I found that std::stod is locale dependent. It is a thinly veiled wrapper of std::strtod which does mention the locale dependance. To fix this issue I did the following:

  • Create a method with the same signature as std::stod in the IO.h class
  • Implement a string to decimal conversion that is invariate to the system specified locale (the two potential methods are described below)
  • Replace all calls to std::stod with the new IO::stod method

Testing I've completed

  • Tested against the sample here and it is working

Looking for feedback on...

  • Hoping that many people can test/run
  • Not sure how the imports should be formatted and other code formatting things. Is there a style guide or preferred linter?

Fast_Float

  • Involves adding a (header's only) lib to the project (already done with cmake in the PR)
  • It is used in chromium, updated within the last month, and has 1.5k stars so it has all the good signs of a quality library to include
  • It is headers only and privately used inside a class so it does not require an export or any modifications to user/post-compile code
  • It has very promising speed claims for large read/writes
    image
    Source

The two options for replacing std::stod are:

  • Parsing with the isstream method (standard c++):
    double result;
    std::istringstream iss(__str);
    iss.imbue(std::locale(_locale));
    iss >> result;
  • with fast_float
    double result;
    fast_float::from_chars(__str.data(), __str.data()+__str.size(), result);

In my preliminary testing on my machine against the three methods I got these results for program runtime:

fast_float
---
Runtime = 153526094[µs] = 153 seconds

istringstream
---
Runtime = 156114023[µs] = 156 seconds

std::stod (not a solution because of locale dependence but nice to compare to) 
---
Runtime = 159572281[µs] = 159 seconds

I conducted these tests using the sample program linked above. I don't see a reason to not use the fast library but if there is opposition to including another library the istringstream method would also work.

CHANGELOG.md (choose one)

  • When things are reviewed and ready I can also add some more comments
  • Will update once it is reviewed and approved to avoid continually rebasing against changelog conflicts while it is under review.

This change is Reviewable

@alexbeattie42 alexbeattie42 marked this pull request as draft October 18, 2024 18:04
@alexbeattie42 alexbeattie42 changed the title Minimum stod change [WIP] Minimum stod change Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant