Text format (e.g. IridasCube) parsing optimizations #2074

aras-p · 2024-10-01T10:31:59Z

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:

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).
- Turns out that 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.
- 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.

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]>

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]>

Signed-off-by: Aras Pranckevicius <[email protected]>

remia · 2024-10-02T10:51:20Z

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.

aras-p · 2024-10-02T10:54:26Z

@remia thanks! I admit I did not expect that this PR would need so many failed iterations. <charconv> is a tough beast, it seems :)

Signed-off-by: Aras Pranckevicius <[email protected]>

remia

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!

cozdas · 2024-10-16T19:34:34Z

src/utils/NumberUtils.h

+// 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


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.

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.

aras-p · 2024-10-17T04:05:31Z

Doing some test on an Intel MBP, Xcode 16, it seems to still not support from_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 <charconv>

cozdas · 2024-11-21T22:11:15Z

src/utils/StringUtils.h

@@ -123,7 +137,7 @@ inline std::string RightTrim(std::string str, char c)
 inline std::string RightTrim(std::string str)


Sorry for delayed response.
I wanted to double check that those changes won't break the other parts of the library where we need to deal with unicode strings but then pulled into different directions.

Are we sure that this "unsigned char" based IsSpace() function can operate reliably with UTF-8 encoded, potentially multi-byte characters? Especially the RightTrim() feels dangerous which is used in places like GetAbsoluteSearchPaths() etc.

Hmm with a closer look, that doesn't seem to be using this overload. So the current use-case wise we're safe I guess.

Yes this would work correctly with UTF-8 too, as in it would detect ASCII spaces just fine. UTF-8 is a full superset of 7-bit ASCII; any "longer than one byte" character bytes in there all have the highest bit set.

Yup, you are right. MSB can be 0 only for the first byte of the multi-byte character.
All clear!

cozdas

Looks good.

aras-p · 2024-12-06T12:00:22Z

Now that two reviews have been done, is there something expected from my side to move forward with this PR?

doug-walker · 2024-12-06T17:15:52Z

@aras-p , apologies for the delay in getting this merged. Our CI system is currently having some problems which is causing checks to fail, but I don't think we will need you to do anything more on this one.

It's great to have your participation on OCIO -- we'll try to get your next PR merged more quickly! ;)

aras-p force-pushed the cube-parser-opt branch from dab9471 to 769e887 Compare October 1, 2024 10:35

aras-p added 3 commits October 1, 2024 19:54

Fix test failures (char can be signed, doh)

74b465a

Signed-off-by: Aras Pranckevicius <[email protected]>

Tests: add unit test coverage for NumberUtils::from_chars directly

8f5be6a

Currently it was only tested indirectly via XMLReaderUtils_tests and file format tests Signed-off-by: Aras Pranckevicius <[email protected]>

Fix from_chars in C++17 code path to understand hex prefix (0x) and s…

8428381

…kip optional whitespace To match the pre-C++17 behavior that was there before Signed-off-by: Aras Pranckevicius <[email protected]>

aras-p force-pushed the cube-parser-opt branch from 0e81b9b to 8428381 Compare October 2, 2024 08:08

aras-p changed the title ~~Iridas .cube parser optimizations~~ Text format (e.g. IridasCube) parsing optimizations Oct 2, 2024

Fix detection of <charconv> float from_chars availability

3353ac4

Signed-off-by: Aras Pranckevicius <[email protected]>

aras-p force-pushed the cube-parser-opt branch from 4ecef8f to 3353ac4 Compare October 2, 2024 09:54

Tests: Fix missing <limits> include for gcc

3f65bb3

Signed-off-by: Aras Pranckevicius <[email protected]>

Tests: fix uninitialized variable warning-as-error on gcc

53fbca9

Signed-off-by: Aras Pranckevicius <[email protected]>

doug-walker requested a review from cozdas October 11, 2024 05:17

remia approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'main' into cube-parser-opt

27f8ab7

cozdas reviewed Oct 16, 2024

View reviewed changes

cozdas reviewed Nov 21, 2024

View reviewed changes

cozdas approved these changes Nov 22, 2024

View reviewed changes

Merge branch 'main' into cube-parser-opt

3a3e7e5

Merge branch 'main' into cube-parser-opt

8d90274

doug-walker merged commit f2dc147 into AcademySoftwareFoundation:main Dec 7, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text format (e.g. IridasCube) parsing optimizations #2074

Text format (e.g. IridasCube) parsing optimizations #2074

aras-p commented Oct 1, 2024 •

edited

Loading

remia commented Oct 2, 2024

aras-p commented Oct 2, 2024

remia left a comment •

edited

Loading

cozdas Oct 16, 2024

aras-p Oct 17, 2024

aras-p commented Oct 17, 2024

cozdas Nov 21, 2024

cozdas Nov 22, 2024

aras-p Nov 22, 2024

cozdas Nov 22, 2024

cozdas left a comment •

edited

Loading

aras-p commented Dec 6, 2024

doug-walker commented Dec 6, 2024

		@@ -123,7 +137,7 @@ inline std::string RightTrim(std::string str, char c)
		inline std::string RightTrim(std::string str)

Text format (e.g. IridasCube) parsing optimizations #2074

Text format (e.g. IridasCube) parsing optimizations #2074

Conversation

aras-p commented Oct 1, 2024 • edited Loading

remia commented Oct 2, 2024

aras-p commented Oct 2, 2024

remia left a comment • edited Loading

Choose a reason for hiding this comment

cozdas Oct 16, 2024

Choose a reason for hiding this comment

aras-p Oct 17, 2024

Choose a reason for hiding this comment

aras-p commented Oct 17, 2024

cozdas Nov 21, 2024

Choose a reason for hiding this comment

cozdas Nov 22, 2024

Choose a reason for hiding this comment

aras-p Nov 22, 2024

Choose a reason for hiding this comment

cozdas Nov 22, 2024

Choose a reason for hiding this comment

cozdas left a comment • edited Loading

Choose a reason for hiding this comment

aras-p commented Dec 6, 2024

doug-walker commented Dec 6, 2024

aras-p commented Oct 1, 2024 •

edited

Loading

remia left a comment •

edited

Loading

cozdas left a comment •

edited

Loading