-
Notifications
You must be signed in to change notification settings - Fork 461
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
Improve the speed of parsing floating-point numbers #1496
Improve the speed of parsing floating-point numbers #1496
Conversation
ee60b48
to
ae53620
Compare
Force-pushed again because of a typo in one of the issue numbers. |
Thanks for the contribution @amyspark, great to see some progress on this long standing issue ! I have yet to test that on actual files, though looking at the changes I already had questions (I have not followed all discussions on the topic over the years, trying to understand):
|
Hey @remia !
Anything that is not
I believe so; iirc @hodoulp rejected the stringstream+imbuelocale approach because it is notoriously slower compared to a plain sscanf. And using C++17
It is tricky because you need to change the running thread's locale through your C runtime before being able to reproduce it, e.g. with |
Since the parsing utils already pay the cost of |
Thanks for your contribution @amyspark ! I added this PR to the TSC meeting agenda tomorrow for more discussion. |
Use the Using the command Using the command |
Following my first investigations, here are some thoughts:
|
❗ As agreed at the last TSC meeting, all the third-party library dependencies must remain external (when used as-is) and be included using a dedicated |
I was not present at the meeting, so I would like to take the opportunity to address these points below.
The library was fetched and committed as is from the release: https://github.com/fastfloat/fast_float/releases/tag/v3.1.0. https://github.com/fastfloat/fast_float/tree/v3.1.0#using-as-single-header lists the amalgamation process. I'm afraid the upstream doesn't list SHA256 hashes, so I cannot provide more assurances regarding that.
The list of issues in this PR shows that this bug has existed for almost a decade, which is a very long time in our field. This feature in particular not only requires a fairly new standard (for instance, we at Krita only moved to 14 last year), it also needs a very recent compiler. According to cppreference, to date, only GCC 11 and MSVC 2017 support
I will port it to an external dependency in the next force push.
stringstream is unavoidable until OpenColorIO migrates its formatting utilities to fmt, because |
e4ae2f5
to
c4dddb7
Compare
I've now pushed the requested fixes. @hodoulp, Spi1D now also uses |
@amyspark OpenColorIO default C++ standard is still C++11. We recently agreed to move to C++14 to stay close to the default C++ version supported by the ecosystem (refer to pull request #1508) but C++11 must still compile. So, there is no way to directly use On the other hand, we are also extremely careful when adding a mandatory third-party library and how to integrate it in the project to keep the build easy for any external contributors or integrators. The last commits are fixing all of these points: 1) BTW During my tests around the performance aspect, I also double-checked for the precision which seems the same i.e. no loss. ✅ From the On my previous comment, I forgot to mention the unit test aspect. Even if the performance improvement is great the |
Indeed, to_chars is actually a C++17 feature, not even C++14. |
@lgritz Yeah, however it is usable though GCC and Clang in C++14 and C++11 (although they will trigger a |
@amyspark Certainly. Though you'd need to map out which versions of those compilers support it (all the way back to the oldest versions that support C++14? really?) and what about MSVS? |
@lgritz MSVC is standards compliant, i.e. it only allows |
I think we should avoid compiler flags to enable features from 'higher' C++ versions. That would require much more CI builds (i.e. to test all combinations of compilers, flags, etc.) and also increase the risk of problems on not officially support platforms, compilers, etc. |
Here are my new numbers demonstrating a major improvement for
Footnotes
|
I checked the changes, and I have few comments:
|
Fixed in the upcoming commits.
Here I am running into three semantic issues with
(from cppreference)
It seems the CLF format relies explicitly on the plus sign and on the ability of
fast_float doesn't have support for hex numbers at all. I'm going to push a new commit showing how to work around it, and let you decide if the workaround is good enough for production purposes. |
❗ Not supporting numbers like The side-effect of this finding is the demonstration of the lake of unit tests around the |
Here are few improvements before having the final answer about the go vs. no go for the library in OpenColorIO:
|
The new numbers are:
Comment: The performance hit for the Footnotes
|
@hodoulp, I understood that the library was to be built from source, as is done with the rest of the dependencies? |
In general, the third-party libraries are built from sources. But this one offers a header only solution which will reduce compile time. So, we should take it. |
No. As we try to keep the build number under control (i.e. there is already lot of builds) It would be great to only add one or two new entries. |
e88bfc0
to
e742c40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to put some time investigating the last point: parsing difference between fast_float::from_chars
and ::strtof_l
in order to remove the temp
string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks great @amyspark, I made a few minor points that are mostly questions and not issues. Also wondering if we can add a simple test case with, say simulating a French locale system and checking the behaviour is as expected (though I'm not sure the exact scope this test should cover). Has this been discussed and sorted already?
fcb15ab
to
e7c3aac
Compare
Note: I believe the additional checks showing up in the list are due to the renaming of the jobs in the CI workflow file. We need to update the required list in the repository settings. Or at least that would be my guess. |
e7c3aac
to
1c07fd8
Compare
The pull request now seems ready but I would like to double-check the performance of this final version before approving. Great job @amyspark |
As discussed yesterday here is some performance numbers (from different hardware) on the 3 major platforms using the new macOS 11.6 AppleClang 13.0 CXX14
Windows 10 MSVC 19.16.27045.0 CXX14
Linux CentOS 7.6 GCC 9.3.1 CXX14
Footnotes
|
@amyspark The add of a library dependency in the 'core' library is always a challenging discussion because of all the potential impacts on short and long term for OpenColorIO maintainers but also for package managers, external contributors, etc. In that case, the goal is to add these changes to the coming 2.1.1 release so, we cannot add a new dependency. The patch version 2.1.1 is planned for next week, will you have the time to cleanup the code? |
@hodoulp What would be, more precisely, the changes you'd like in this MR? And what should be moved to another commit or MR? |
Please remove the |
2c21892
to
5cc9446
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work amyspark and for your patience during the long process of finding a solution that meets the many requirements/constraints!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work @amyspark! Just a minor point for coherence about the sscanf
on Windows but all good to merge for me as well regardless.
This commit adds support for parsing numbers without being influenced by the current system locale. We implement a from_chars shim that forwards the call to strto*_l along with a statically initialized locale constant. Fixes AcademySoftwareFoundation#297 Fixes AcademySoftwareFoundation#379 Fixes AcademySoftwareFoundation#1322 Co-Authored-By: Patrick Hodoul <[email protected]> Signed-off-by: L. E. Segovia <[email protected]>
5cc9446
to
2785410
Compare
This commit adds support for parsing numbers without being influenced by the current system locale. We implement a from_chars shim that forwards the call to strto*_l along with a statically initialized locale constant. Fixes #297 Fixes #379 Fixes #1322 Co-Authored-By: Patrick Hodoul <[email protected]> Signed-off-by: L. E. Segovia <[email protected]>
This commit adds support for parsing numbers without being influenced by the current system locale. We implement a from_chars shim that forwards the call to strto*_l along with a statically initialized locale constant. Fixes #297 Fixes #379 Fixes #1322 Co-Authored-By: Patrick Hodoul <[email protected]> Signed-off-by: amyspark <[email protected]>
This commit adds support for parsing numbers without being influenced by the current system locale. We implement a from_chars shim that forwards the call to strto*_l along with a statically initialized locale constant. Fixes #297 Fixes #379 Fixes #1322 Co-Authored-By: Patrick Hodoul <[email protected]> Signed-off-by: amyspark <[email protected]> Co-authored-by: amyspark <[email protected]>
This MR brings in the amalgamated header of the fast_float library, and applies it to our uses of sscanf with %f. This library is locale-free, and does not incur the performance penalty of using a thread locale RAII class.
I have also included the relevant entries to THIRD_PARTY, and added a succint Readme listing the library version and its source, in the style of sampleicc.
Fixes #297
Fixes #379
Fixes #1322