Skip to content

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

Merged
doug-walker merged 10 commits into
AcademySoftwareFoundation:mainfrom
aras-p:cube-parser-opt
Dec 7, 2024
Merged

Text format (e.g. IridasCube) parsing optimizations#2074
doug-walker merged 10 commits into
AcademySoftwareFoundation:mainfrom
aras-p:cube-parser-opt

Conversation

@aras-p
Copy link
Copy Markdown
Contributor

@aras-p aras-p commented Oct 1, 2024

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 <aras@nesnausk.org>
aras-p added 3 commits October 1, 2024 19:54
Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Currently it was only tested indirectly via XMLReaderUtils_tests and
file format tests

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
…kip optional whitespace

To match the pre-C++17 behavior that was there before

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
@aras-p aras-p changed the title Iridas .cube parser optimizations Text format (e.g. IridasCube) parsing optimizations Oct 2, 2024
Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
@remia
Copy link
Copy Markdown
Collaborator

remia commented Oct 2, 2024

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
Copy link
Copy Markdown
Contributor Author

aras-p commented Oct 2, 2024

@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 <aras@nesnausk.org>
@doug-walker doug-walker requested a review from cozdas October 11, 2024 05:17
Copy link
Copy Markdown
Collaborator

@remia remia left a 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!

Comment thread 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
Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Contributor Author

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.

@aras-p
Copy link
Copy Markdown
Contributor Author

aras-p commented Oct 17, 2024

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>

Comment thread src/utils/StringUtils.h
@@ -123,7 +137,7 @@ inline std::string RightTrim(std::string str, char c)
inline std::string RightTrim(std::string str)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@cozdas cozdas left a comment

Choose a reason for hiding this comment

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

Looks good.

@aras-p
Copy link
Copy Markdown
Contributor Author

aras-p commented Dec 6, 2024

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

@doug-walker
Copy link
Copy Markdown
Collaborator

@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! ;)

@doug-walker doug-walker merged commit f2dc147 into AcademySoftwareFoundation:main Dec 7, 2024
doug-walker added a commit to autodesk-forks/OpenColorIO that referenced this pull request Dec 12, 2024
…oundation#2074)

* Iridas .cube and other text format parsing optimizations

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 <aras@nesnausk.org>

* Fix test failures (char can be signed, doh)

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

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

Currently it was only tested indirectly via XMLReaderUtils_tests and
file format tests

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Fix from_chars in C++17 code path to understand hex prefix (0x) and skip optional whitespace

To match the pre-C++17 behavior that was there before

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Fix detection of <charconv> float from_chars availability

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Tests: Fix missing <limits> include for gcc

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

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

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

---------

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit f2dc147)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
doug-walker added a commit that referenced this pull request Dec 12, 2024
* Match ACES 2.0 shader resource suffix format to regular luts. (#2077)

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
(cherry picked from commit 59d6a86)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Build: enable parallel source file compilation on MSVC (#2072)

By default Visual Studio projects do not compile source files in
parallel. Add "/MP" flag to enable that.

From scratch OCIO build on Ryzen 5950X, VS2022: 580sec -> 208sec.
Most of remaining single-threaded time is cloning external libraries
or building external libraries (most of them don't do parallel
compilation either). But within building OCIO itself, CPU usage
goes close to 100% instead of 5%.

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 707734d)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Move GPU LUT files into the tests dir (#2069)

Instead of creating temporary LUT files for GPU tests,
read them from the testdata directory instead.
This removes the need to clean them up after each run.

Signed-off-by: Ananth Bhaskararaman <ananth.b@qubecinema.com>
(cherry picked from commit b3900df)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Photoshop now supports OCIO (#2084)

* ocio gm in photoshop

Signed-off-by: Dave Sawyer <kingsawyer@gmail.com>

* Update supported_apps.yml

Signed-off-by: Dave Sawyer <kingsawyer@gmail.com>

* ps and fnordware text

Signed-off-by: Dave Sawyer <kingsawyer@gmail.com>

---------

Signed-off-by: Dave Sawyer <kingsawyer@gmail.com>
(cherry picked from commit 81c07fd)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Update doc building instructions (#2085)

* Update doc building instructions

Signed-off-by: Craig Zerouni <czerouni@gmail.com>

* Remove stray comment line

Signed-off-by: Craig Zerouni <czerouni@gmail.com>

---------

Signed-off-by: Craig Zerouni <czerouni@gmail.com>
(cherry picked from commit 800efd8)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* add Disguise to list of supported applications (#2088)

Signed-off-by: Chris Nash <chris.nash@disguise.one>
Co-authored-by: zachlewis <zachlewis@users.noreply.github.com>
(cherry picked from commit 9bc5b8e)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Add `DISPLAY - CIE-XYZ-D65_to_DisplayP3-HDR` builtin transform. (#2095)

(cherry picked from commit f37ca55)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix broken Linux and Mac CI workflow (#2102)

* Fix broken Linux and Mac CI workflow

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Update job name

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

---------

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit c7ad2a9)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

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

* Iridas .cube and other text format parsing optimizations

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 <aras@nesnausk.org>

* Fix test failures (char can be signed, doh)

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

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

Currently it was only tested indirectly via XMLReaderUtils_tests and
file format tests

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Fix from_chars in C++17 code path to understand hex prefix (0x) and skip optional whitespace

To match the pre-C++17 behavior that was there before

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Fix detection of <charconv> float from_chars availability

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

* Tests: Fix missing <limits> include for gcc

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

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

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>

---------

Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit f2dc147)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* [2057] - Fix views/display submenus in OCIODisplay app (#2068)

* fix views/display submenus

Signed-off-by: Hannah <hannah.mk23@gmail.com>

* fix global variable name

Signed-off-by: Hannah <hannah.mk23@gmail.com>

---------

Signed-off-by: Hannah <hannah.mk23@gmail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 5c2fa57)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix bit-depth attr for inv luts for CTF writing (#2090)

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit dafefe6)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fixed range function error in nuke colorlookup examples (#2096)

* Error found: ocio_to_colorlookup_all.py

Signed-off-by: Seif Ashraf <seifibrahim32@GMail.com>

* Update colorlookup_to_spi1d.py

Signed-off-by: Seif Ashraf <seifibrahim32@GMail.com>

* Update ocio_to_colorlookup_rgb.py

Signed-off-by: Seif Ashraf <seifibrahim32@GMail.com>

---------

Signed-off-by: Seif Ashraf <seifibrahim32@GMail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 0f06f04)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Replaced DX11 HLSL shading language with the more accurate shading model 5.0 (SM_5_0). (#2078)

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 3132579)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Adsk Contrib - Optimizer should detect pair inverses before combining multiple op pairs (#2104)

* Fix opt combine issue

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Improve gamma test

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Add check for DisplayP3-HDR built-in

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

---------

Signed-off-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 1890b7d)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Fix Python wheel macOS workflow and make OCIO_PYTHON_LOAD_DLLS_FROM_PATH opt-in (#2106)

Signed-off-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Doug Walker <doug.walker@autodesk.com>
(cherry picked from commit 6fa40a4)
Signed-off-by: Doug Walker <doug.walker@autodesk.com>

* Increment library version to 2.4.1

Signed-off-by: Doug Walker <doug.walker@autodesk.com>

---------

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Aras Pranckevicius <aras@nesnausk.org>
Signed-off-by: Ananth Bhaskararaman <ananth.b@qubecinema.com>
Signed-off-by: Dave Sawyer <kingsawyer@gmail.com>
Signed-off-by: Craig Zerouni <czerouni@gmail.com>
Signed-off-by: Chris Nash <chris.nash@disguise.one>
Signed-off-by: Hannah <hannah.mk23@gmail.com>
Signed-off-by: Seif Ashraf <seifibrahim32@GMail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: Éric Renaud-Houde <eric.renaud.houde@gmail.com>
Co-authored-by: Aras Pranckevičius <aras@nesnausk.org>
Co-authored-by: Ananth <srv+gh@kedi.dev>
Co-authored-by: Dave Sawyer <kingsawyer@gmail.com>
Co-authored-by: Craig Zerouni <czerouni@gmail.com>
Co-authored-by: Chris Nash <chris.nash@disguise.one>
Co-authored-by: zachlewis <zachlewis@users.noreply.github.com>
Co-authored-by: Thomas Mansencal <thomas.mansencal@gmail.com>
Co-authored-by: Rémi Achard <remiachard@gmail.com>
Co-authored-by: hannahmkrasnick <81137034+hannahmkrasnick@users.noreply.github.com>
Co-authored-by: Seif Ashraf <seifibrahim32@GMail.com>
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.

4 participants