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

Huffman performance regression #1479

Closed
peterurbanec opened this issue Jul 4, 2023 · 4 comments
Closed

Huffman performance regression #1479

peterurbanec opened this issue Jul 4, 2023 · 4 comments

Comments

@peterurbanec
Copy link
Contributor

PR #1323 introduces a nested #ifdef check that results in a performance regression on Linux systems that use clang as the compiler. This is because the check for __clang__ succeeds, but the nested check for __APPLE__ fails. As a result, the elif case is not taken on Linux.

Suggested fix:

diff --git a/src/lib/OpenEXR/ImfFastHuf.cpp b/src/lib/OpenEXR/ImfFastHuf.cpp
index 0fa9f315..82cf2ed2 100644
--- a/src/lib/OpenEXR/ImfFastHuf.cpp
+++ b/src/lib/OpenEXR/ImfFastHuf.cpp
@@ -14,14 +14,11 @@
 // Static enabling/disabling the fast huffman decode


-#if defined(__clang__)
+#if defined(__APPLE__) && defined(__clang__)
 //
 // Enabled for clang on Apple platforms (tested):
 //
-
-#    if defined(__APPLE__)
-#        define OPENEXR_IMF_ENABLE_FAST_HUF_DECODER
-#    endif
+#    define OPENEXR_IMF_ENABLE_FAST_HUF_DECODER

 #elif defined(__INTEL_COMPILER) || defined(__GNUC__)
 //
@lgritz
Copy link
Contributor

lgritz commented Jul 4, 2023

This fix looks right to me, good catch. Can you please submit it as a real PR so it will run the CI and we can merge it easily with one click?

@peterurbanec
Copy link
Contributor Author

I'll submit a PR.

At this stage I don't have an executed CCLA, but given the trivial nature of the change, that may not be a
show stopper...

peterurbanec added a commit to peterurbanec/openexr that referenced this issue Jul 4, 2023
PR AcademySoftwareFoundation#1323 introduces a nested #ifdef check that results in a performance regression on Linux systems that use the clang compiler. This is because the check for __clang__ succeeds, but the nested check for __APPLE__ fails. As a result, the elif case is not taken on Linux.

Fixes issue AcademySoftwareFoundation#1479
peterurbanec added a commit to peterurbanec/openexr that referenced this issue Jul 4, 2023
PR AcademySoftwareFoundation#1323 introduces a nested #ifdef check that results in a performance
regression on Linux systems that use the clang compiler. This is because
the check for __clang__ succeeds, but the nested check for __APPLE__
fails. As a result, the elif case is not taken on Linux.

Fixes issue AcademySoftwareFoundation#1479


Signed-off-by: Peter Urbanec <[email protected]>
peterurbanec added a commit to peterurbanec/openexr that referenced this issue Jul 4, 2023
PR AcademySoftwareFoundation#1323 introduces a nested #ifdef check that results in a performance
regression on Linux systems that use the clang compiler. This is because
the check for __clang__ succeeds, but the nested check for __APPLE__
fails. As a result, the elif case is not taken on Linux.

Fixes issue AcademySoftwareFoundation#1479


Signed-off-by: Peter Urbanec <[email protected]>
cary-ilm pushed a commit that referenced this issue Jul 9, 2023
PR #1323 introduces a nested #ifdef check that results in a performance
regression on Linux systems that use the clang compiler. This is because
the check for __clang__ succeeds, but the nested check for __APPLE__
fails. As a result, the elif case is not taken on Linux.

Fixes issue #1479

Signed-off-by: Peter Urbanec <[email protected]>
@cary-ilm
Copy link
Member

I went ahead and merged the PR. We normally do require a signed CLA but for such a simple fix we'll waive it, especially since the DCO is in place.

@peterurbanec
Copy link
Contributor Author

Thanks. Incidentally the CCLA paperwork just came through, so the contribution is now covered.

cary-ilm pushed a commit to cary-ilm/openexr that referenced this issue Jul 25, 2023
…ndation#1480)

PR AcademySoftwareFoundation#1323 introduces a nested #ifdef check that results in a performance
regression on Linux systems that use the clang compiler. This is because
the check for __clang__ succeeds, but the nested check for __APPLE__
fails. As a result, the elif case is not taken on Linux.

Fixes issue AcademySoftwareFoundation#1479

Signed-off-by: Peter Urbanec <[email protected]>
cary-ilm pushed a commit that referenced this issue Jul 31, 2023
PR #1323 introduces a nested #ifdef check that results in a performance
regression on Linux systems that use the clang compiler. This is because
the check for __clang__ succeeds, but the nested check for __APPLE__
fails. As a result, the elif case is not taken on Linux.

Fixes issue #1479

Signed-off-by: Peter Urbanec <[email protected]>
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

No branches or pull requests

3 participants