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

BUG: Update VTK to to fix vtkSMPToolsAPI static initialization order issue #7453

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Dec 8, 2023

This addresses a crash related to the static initialization order 'fiasco' in vtkSMPToolsAPI. It ensures proper deletion of the vtkSMPToolsAPI singleton once the last translation unit referencing it has been unloaded.

This update ensures that the SMP backend is cleaned up at the appropriate time, fixing the following macOS tests that started to fail following commit 28dca75 (ENH: Enable TBB as the default VTK SMP implementation on all platforms):

  • py_nomainwindow_SegmentationWidgetsTest1
  • py_sceneImport2428
  • py_SegmentStatistics
  • py_UtilTest

List of changes:

$ git shortlog  4bfb0f049a..d1727b8ce6 --no-merges Jean-Christophe Fillion-Robin (2):
      [Backport MR-10751] BUG: Resolve crash by fixing vtkSMPToolsAPI static initialization order issue
      [Backport MR-10751] ENH: Simplify vtkSMPToolsAPIInitialize managing pointer and counter locally

Related

@jcfr jcfr linked an issue Dec 8, 2023 that may be closed by this pull request
@jcfr jcfr requested a review from pieper December 8, 2023 16:47
@jcfr
Copy link
Member Author

jcfr commented Dec 8, 2023

The topic has been locally checkout on our Linux, macOS and Windows factories.

Once build and tests are confirmed to be working, I will follow up.

@jcfr jcfr added the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Dec 8, 2023
pieper
pieper previously approved these changes Dec 8, 2023
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

Whew! 👍

@jcfr
Copy link
Member Author

jcfr commented Dec 8, 2023

Update related to build and testing of this pull request:

  • on macOS (factory computron) ✅
  • on Linux (factory metroplex) ✅
  • on Windows (factory metroplex) ⏳

On the three factories the branch corresponding to this pull request was checked out and the Slicer project rebuilt from the top-level.

"factory" describes the build machine used to build and test the official Slicer packages.

macOS testing

svc-dashboard@computron Slicer-build % /Users/svc-dashboard/D/Support/CMake-3.22.1.app/Contents/bin/ctest -R "py_nomainwindow_SegmentationWidgetsTest1|py_sceneImport2428|py_SegmentStatistics|py_UtilTest" 
Test project /D/P/A/Slicer-build
    Start 377: py_nomainwindow_SegmentationWidgetsTest1
1/4 Test #377: py_nomainwindow_SegmentationWidgetsTest1 ...   Passed  160.83 sec
    Start 444: py_SegmentStatistics
2/4 Test #444: py_SegmentStatistics .......................   Passed   57.22 sec
    Start 611: py_sceneImport2428
3/4 Test #611: py_sceneImport2428 .........................   Passed   13.85 sec
    Start 626: py_UtilTest
4/4 Test #626: py_UtilTest ................................   Passed   14.17 sec

100% tests passed, 0 tests failed out of 4

Total Test time (real) = 246.15 sec

Linux testing

This is the only platform where I explicitly indicate the branch is checked out and the project rebuilt.

$ cd ~/Dashboards/Slicer

$ ~/bin/slicer-buildenv-qt5-centos7-latest bash 

then

$ cd /work/Preview/Slicer-0

$ git remote add jcfr https://github.com/Slicer/Slicer
$ git fetch jcfr
$ git checkout backport-vtk-fix-vtkSMPToolsAPI-crash-ensuring-proper-static-initialization

$ cd /work/Preview/Slicer-0-build/Slicer-build
$ ninja

$ export QT_QPA_PLATFORM=offscreen
$ Xvfb :99 -screen 0 1024x768x24 +extension GLX +render -noreset &

$ cd /work/Preview/Slicer-0-build/Slicer-build

$ ctest -R "py_nomainwindow_SegmentationWidgetsTest1|py_sceneImport2428|py_SegmentStatistics|py_UtilTest" 
Test project /work/Preview/Slicer-0-build/Slicer-build
    Start 377: py_nomainwindow_SegmentationWidgetsTest1
1/4 Test #377: py_nomainwindow_SegmentationWidgetsTest1 ...   Passed   27.82 sec
    Start 444: py_SegmentStatistics
2/4 Test #444: py_SegmentStatistics .......................   Passed   53.47 sec
    Start 611: py_sceneImport2428
3/4 Test #611: py_sceneImport2428 .........................   Passed   11.63 sec
    Start 626: py_UtilTest
4/4 Test #626: py_UtilTest ................................   Passed   14.50 sec

100% tests passed, 0 tests failed out of 4

Total Test time (real) = 107.50 sec

Windows testing

⏳ In progress...

@jcfr
Copy link
Member Author

jcfr commented Dec 8, 2023

re: Windows

D:\D\P\S-0-build\Slicer-build>C:\cmake-3.22.1\bin\ctest.exe -C Release -R "py_nomainwindow_SegmentationWidgetsTest1|py_sceneImport2428|py_SegmentStatistics|py_UtilTest"
Test project D:/D/P/S-0-build/Slicer-build
    Start 377: py_nomainwindow_SegmentationWidgetsTest1
1/4 Test #377: py_nomainwindow_SegmentationWidgetsTest1 ...***Failed    8.46 sec
    Start 444: py_SegmentStatistics
2/4 Test #444: py_SegmentStatistics .......................***Failed    0.26 sec
    Start 609: py_sceneImport2428
3/4 Test #609: py_sceneImport2428 .........................***Failed    0.25 sec
    Start 624: py_UtilTest
4/4 Test #624: py_UtilTest ................................***Failed    0.19 sec

0% tests passed, 4 tests failed out of 4

Total Test time (real) =   9.54 sec

The following tests FAILED:
        377 - py_nomainwindow_SegmentationWidgetsTest1 (Failed)
        444 - py_SegmentStatistics (Failed)
        609 - py_sceneImport2428 (Failed)
        624 - py_UtilTest (Failed)

@jcfr
Copy link
Member Author

jcfr commented Dec 8, 2023

I am further improving the VTK patch to also ensures the statics used in the each backend are properly handled.

…issue

This addresses a crash related to the static initialization order 'fiasco'
in vtkSMPToolsAPI. It ensures proper deletion of the vtkSMPToolsAPI singleton
once the last translation unit referencing it has been unloaded.

It also ensures backend-specific statics are properly initialized.
when the first translation unit is loaded.

This update ensures that the SMP backend is cleaned up at the appropriate
time, fixing the following macOS tests that started to fail following commit
28dca75 (ENH: Enable TBB as the default VTK SMP implementation on all platforms):
* `py_nomainwindow_SegmentationWidgetsTest1`
* `py_sceneImport2428`
* `py_SegmentStatistics`
* `py_UtilTest`

List of changes:

$ git shortlog 4bfb0f049a..46201478cd --no-merges
Jean-Christophe Fillion-Robin (3):
      [Backport MR-10751] BUG: Resolve crash by fixing vtkSMPToolsAPI static initialization order issue
      [Backport MR-10751] ENH: Simplify vtkSMPToolsAPIInitialize managing pointer and counter locally
      [Backport MR-10751] BUG: Resolve crash by fixing initialization of backend-specific statics
@jcfr jcfr force-pushed the backport-vtk-fix-vtkSMPToolsAPI-crash-ensuring-proper-static-initialization branch from c29e59a to 650cae1 Compare December 8, 2023 22:43
@jcfr
Copy link
Member Author

jcfr commented Dec 9, 2023

Et voila, after updating MR-107511 with the additional commit BUG: Resolve crash by fixing initialization of backend-specific statics, build and test re successful on all platform ✅

Footnotes

  1. https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10751

@jcfr jcfr merged commit c39049a into Slicer:main Dec 9, 2023
5 checks passed
@jcfr jcfr deleted the backport-vtk-fix-vtkSMPToolsAPI-crash-ensuring-proper-static-initialization branch December 9, 2023 06:44
@jcfr
Copy link
Member Author

jcfr commented Dec 9, 2023

Manually re-triggered nightly builds look good 🎉

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Address macOS test failures related to TBB backend enabled by default
2 participants