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

Update Catch2 to version 2.13.10 #1566

Merged

Conversation

ashwinbhat
Copy link
Contributor

@ashwinbhat ashwinbhat commented Oct 12, 2023

  • Update Catch2 version to v2.13.10
  • Catch2 v2.13.0 supports BENCHMARK tests.
  • A new BENCHMARK test "GenShader: ShaderGen Performance" to benchmark document load, validation and shadergen times

Update to Catch v2.13.10 to use BENCHMARK tests.
Add a new "GenShader: ShaderGen Performance" test to measure time take to load, validate and generate shaders
@ashwinbhat ashwinbhat changed the title Update to Catch2 version Update Catch2 version Oct 12, 2023
@jstone-lucasfilm
Copy link
Member

This looks very promising, thanks @ashwinbhat!

Looking at the build times, though, I see that our Dynamic Analysis tests take quite a while to complete the new benchmark step.

Would it be possible to exclude the benchmark step from Dynamic Analysis builds, e.g. by setting a preprocessor flag when MATERIALX_DYNAMIC_ANALYSIS is enabled in a build, and checking for this before running this specific test?

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/CMakeLists.txt#L56C8-L56C34

ashwinbhat and others added 2 commits October 16, 2023 13:18
Add the Perfromance test only if MATERIALX_DYNAMIC_ANALYSIS is enabled.
Note that test implmentation is always compiled to catch any code/api changes to keep it updated, it is disabled from the default run.
Example output:
100% tests passed, 0 tests failed out of 72
Total Test time (real) = 121.03 sec
The following tests did not run:
         51 - MaterialXTest_PerformanceTest_ShaderGen_and_Validation (Disabled)
@jstone-lucasfilm
Copy link
Member

Thanks for that update, @ashwinbhat, though I'm not sure this is having the desired effect!

Looking at the builds in GitHub Actions, most are taking 25-30 minutes, but the two most recent builds from PR 1566 are taking more than 40 minutes, with the Dynamic Analysis build taking an unusually long time to complete (31 minutes, as compared to 8 minutes for other builds).

One recommendation on the approach to controlling which tests run in Dynamic Analysis builds:

What if we were to set a preprocessor flag named MATERIALX_DYNAMIC_ANALYSIS when the user has selected the option of the same name through CMake, and then simply wrap benchmark tests with a check for this preprocessor flag? That would make it more straightforward to change this behavior in the future, extending it to new tests or removing it from other tests, without needing to synchronize test names in CMake.

Here's an example of this pattern for setting preprocessor flags matching user options in the root CMake file:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/CMakeLists.txt#L168

Enable benchmarks only when MATERIALX_DYNAMIC_ANALYSIS is defined.

MATERIALX_DYNAMIC_ANALYSIS = OFF:
51: Test command: C:\source\MaterialX-adsk-fork-ext\build\bin\Release\MaterialXTest.exe "PerformanceTest: ShaderGen and Validation"
51: Working Directory: C:/source/MaterialX-adsk-fork-ext/build/bin
51: Test timeout computed to be: 10000000
51: Filters: PerformanceTest: ShaderGen and Validation
51: 0.000 s: PerformanceTest: ShaderGen and Validation
51: ===============================================================================
51: All tests passed (1 assertion in 1 test case)

With MATERIALX_DYNAMIC_ANALYSIS = ON:
test 51
      Start 51: MaterialXTest_PerformanceTest_ShaderGen_and_Validation

51: Test command: C:\source\MaterialX-adsk-fork-ext\build\bin\Release\MaterialXTest.exe "PerformanceTest: ShaderGen and Validation"
51: Working Directory: C:/source/MaterialX-adsk-fork-ext/build/bin
51: Test timeout computed to be: 10000000
51: Filters: PerformanceTest: ShaderGen and Validation
51:
51: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
51: MaterialXTest.exe is a Catch v2.13.10 host application.
51: Run with -? for options
51:
51: -------------------------------------------------------------------------------
51: PerformanceTest: ShaderGen and Validation
51: -------------------------------------------------------------------------------
51: C:\source\MaterialX-adsk-fork-ext\source\MaterialXTest\MaterialXGenGlsl\GenGlsl.cpp(123)
51: ...............................................................................
51:
51: benchmark name                       samples       iterations    estimated
51:                                      mean          low mean      high mean
51:                                      std dev       low std dev   high std dev
51: -------------------------------------------------------------------------------
51: Load documents, validate and
51: generate shader                                100             1     2.34307 m
51:                                          1.38572 s     1.38087 s     1.39103 s
51:                                         25.8656 ms      23.34 ms     28.815 ms
51:
51:
51: 142.748 s: PerformanceTest: ShaderGen and Validation
51: ===============================================================================
51: All tests passed (8888 assertions in 1 test case)
51:
51/73 Test #51: MaterialXTest_PerformanceTest_ShaderGen_and_Validation .......   Passed  142.94 sec
@ashwinbhat
Copy link
Contributor Author

Hi @jstone-lucasfilm I think I've updated it to follow you suggestion where PerformanceTest: ShaderGen and Validation is run only with MATERIALX_DYNAMIC_ANALYSIS = ON. I see that the last DYNAMIC_ANALYSIS build took 51m 19s. Should we turn it off completely if it's going to hurt PR time and developer productivity

@jstone-lucasfilm
Copy link
Member

@ashwinbhat Oh, I may have miscommunicated my thinking here, and it's only when MATERIALX_DYNAMIC_ANALYSIS is off that we want to run these new benchmarking tests.

In a dynamic analysis build, the compiler inserts additional error-checking instructions into the generated code, allowing us to enforce that conditions such as memory boundary overwrites never occur in our unit tests.

But because these error-checking instructions dramatically slow down runtime performance, we want to omit our new benchmark tests when this mode has been enabled in the compiler, and we can check for this condition using the MATERIALX_DYNAMIC_ANALYSIS preprocessor flag that you've added.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks very close to the mark, and I had just a couple of suggestions for improvements.

source/MaterialXTest/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm 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 to me, thanks @ashwinbhat!

@jstone-lucasfilm jstone-lucasfilm changed the title Update Catch2 version Update Catch2 to version 2.13.10 Oct 24, 2023
@jstone-lucasfilm jstone-lucasfilm merged commit 926ac27 into AcademySoftwareFoundation:main Oct 24, 2023
31 checks passed
@ashwinbhat ashwinbhat deleted the bhata/updateCatch branch October 24, 2023 23:53
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.

2 participants