-
Notifications
You must be signed in to change notification settings - Fork 455
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
Text format (e.g. IridasCube) parsing optimizations #2074
base: main
Are you sure you want to change the base?
Conversation
Primarily driven by a wish to increase performance of parsing .cube files. But the functions that are added or changed are used across parsing of all/most of text based formats. With these changes, parsing "Khronos PBR Neutral" .cube file (5.4MB) on Ryzen 5950X / VS2022 Release build: 167ms -> 123ms. - Add locale independent IsSpace(char). Somewhat similar to 3aab90d, whitespace trimming perhaps should not be locale dependent. - Add IsEmptyOrWhiteSpace() and use that inside ParseUtils::nextline, instead of doing Trim(line).empty(). - Add StringUtils::StartsWith(char) and use that in various parsers that were constructing whole std::string object just to check for a single character. - When building for C++17 or later, NumberUtils can use standard <charconv> from_chars functions (except on Apple platforms, where those are not implemented for floating point types as of Xcode 15). This has advantage of not having to deal with errno or locales. Saves some thread local storage accesses and function calls (e.g. on Windows errno is actually a function call). - There's a CMake setup change that adds /Zc:__cplusplus flag for MSVC; for backwards compat reasons it does not report proper C++ version detection defines otherwise. Signed-off-by: Aras Pranckevicius <[email protected]>
dab9471
to
769e887
Compare
Signed-off-by: Aras Pranckevicius <[email protected]>
Currently it was only tested indirectly via XMLReaderUtils_tests and file format tests Signed-off-by: Aras Pranckevicius <[email protected]>
…kip optional whitespace To match the pre-C++17 behavior that was there before Signed-off-by: Aras Pranckevicius <[email protected]>
0e81b9b
to
8428381
Compare
Signed-off-by: Aras Pranckevicius <[email protected]>
4ecef8f
to
3353ac4
Compare
Signed-off-by: Aras Pranckevicius <[email protected]>
Apologies for the delay in CI @aras-p, we have to manually approve the workflows due to our current GitHub settings. Trying to see if we can update that or merge your other PR which should also work. |
@remia thanks! I admit I did not expect that this PR would need so many failed iterations. |
Signed-off-by: Aras Pranckevicius <[email protected]>
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.
Doing some test on an Intel MBP, Xcode 16, it seems to still not support from_chars(). I did some quick benchmark and found nice speedups to load the same pbrNeutral.cube (from 140ms to 85ms on average to instanciate a Processor with the FileTransform pointing to the cube). Tested on a .spi3d file but this didn't seem to change the timings.
Looks good to me otherwise, thanks for the additional unit tests!
// correctly and the check starts passing with gcc 11, clang 12, | ||
// msvc 2019 (16.4). It correctly does not pass on apple clang 15, | ||
// since it does not implement from_chars for floats. | ||
#if __cpp_lib_to_chars >= 201611L |
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.
Minor but to make sure:
Seems like __cpp_lib_to_chars "feature-test macros" are standardized in C++20, If I understand correctly -in theory- a C++17 compiler may not have this macro defined even if from_chars are implemented, thus we may have a false-negative here. Is that correct?
The worst case it'll fall back to slow path, so that's totally fine but I'm just curious.
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.
In theory, yes. In practice, this was the "least bad" way I found that would work on all of MSVC/clang/gcc, while also working on Apple Clang (in a sense that Apple Clang does not pass the test; from_chars on floats is still not there). In theory there could be some exotic compiler that might have from_chars for floats but not expose __cpp_lib_to_chars
.
Yeah, Apple's Clang/STL keeps on being "special". Some other software (e.g. Blender) works around this whole issue by embedding https://github.com/fastfloat/fast_float and using that instead of |
Primarily driven by a wish to increase performance of parsing .cube files. But the functions that are added or changed are used across parsing of all/most of text based formats.
With these changes, parsing "Khronos PBR Neutral" Iridas .cube file (5.4MB) on Ryzen 5950X / VS2022 Release build: 167ms -> 126ms. Parsing the same file in CLF format: 150ms -> 137ms.
What the PR does is:
IsSpace(char)
. Somewhat similar to 3aab90d, whitespace trimming perhaps should not be locale dependent.IsEmptyOrWhiteSpace()
and use that insideParseUtils::nextline
, instead of doingTrim(line).empty()
.StringUtils::StartsWith(char)
and use that in various parsers that were constructing wholestd::string
object just to check for a single character.NumberUtils
can use standard<charconv>
from_chars
functions (except on Apple platforms, where those are not implemented for floating point types as of Xcode 15). This has advantage of not having to deal witherrno
or locales. Saves some thread local storage accesses and function calls (e.g. on Windowserrno
is actually a function call).NumberUtils::from_chars
was only indirectly covered by tests, and as XML parser utils tests point out,it should be able to handle hex floats and so on. So all that is implemented there, and explicitly covered by
tests/utils/NumberUtils_tests.cpp
./Zc:__cplusplus
flag for MSVC; for backwards compat reasons it does not report proper C++ version detection defines otherwise.