Fix GUI thread deadlock caused by orphaned OpenMP pragmas - #14593
Open
magnesj wants to merge 9 commits into
Open
Fix GUI thread deadlock caused by orphaned OpenMP pragmas#14593magnesj wants to merge 9 commits into
magnesj wants to merge 9 commits into
Conversation
kriben
self-requested a review
August 24, 2026 09:25
kriben
reviewed
Aug 24, 2026
kriben
reviewed
Aug 24, 2026
kriben
requested changes
Aug 24, 2026
…eRegion The pragma had no enclosing parallel region, so it did not parallelize anything and only added confusion. This code caused a deadlock on Windows for the following workflow: 1. import a grid model 2. Import a large summary ensemble with no ESMRY that triggers display of a progress dialog. Make sure the progress dialog is displayed on top of the 3D models -> deadlock
The loops were annotated with #pragma omp for, which is a work-sharing construct that does nothing outside an enclosing parallel region. Replace it with #pragma omp parallel for so the cell loops actually run in parallel.
totalNumberOfConnections was declared, listed in the reduction clause and used to size otherConnections, but it was never incremented. The counting was lost when the loop body was moved into extractConnectionsForFace, so the call has always been reserve( size() + 0 ), which RigConnectionContainer::reserve turns into a no-op. The reserve was also called by every thread on the shared container without synchronization. Removing it eliminates that data race. The explicit barrier is redundant as well, since the omp for construct has no nowait clause and therefore already synchronizes before the merge.
magnesj
force-pushed
the
vizfwk-remove-orphaned-omp-pragmas
branch
from
August 24, 2026 11:39
7b29c97 to
e66c69b
Compare
…metry generation The loop was declared with an ordered clause, but the body contains no ordered region, so the clause only enabled the ordered scheduling machinery without providing any ordering. The intended deterministic vertex order was not achieved either, because the actual synchronization was a critical section, which does not preserve iteration order. Collect vertices in per thread buffers and merge them in thread order after the parallel region, following the pattern already used in RivFaultGeometryGenerator. This removes a lock acquisition per triangle in the innermost polygon loop, and makes the generated geometry identical from run to run.
Each iteration writes to its own element of accumulatedValidValues, which is sized before the loop, so no synchronization is required. The write to curveValues a few lines below uses the same pattern without a critical section. All unnamed critical regions share a single process wide lock, so this also removes contention against unrelated parallel loops.
… critical sections All four cell filter loops guarded a push_back into a shared container with an unnamed critical section, taking a process wide lock for every cell matching the polygon. On large grids with a permissive polygon this can be slower than running single threaded, and it made the resulting cell order vary between runs. Collect the cells in per thread buffers and append them in thread order after the parallel region, following the pattern used elsewhere in the code base. This removes eight critical sections.
Describe the OpenMP 2.0 constraints that follow from building with MSVC /openmp, and why an orphaned work sharing construct can deadlock the application when the master thread is also the Qt GUI thread. Also document the pattern of collecting results in per thread buffers and merging after the parallel region, the convention of naming critical sections, and the rule that exceptions must not escape a structured block.
All unnamed critical regions map to the same implicit name, so a single process wide lock was shared by summary import, geometry generation, grid bounding box computation and NNC merging. Unrelated parallel loops therefore serialized against each other. Give each region a critical_section_ name describing what it protects. The two regions guarding RifOpmCommonEclipseSummary::sm_createdEsmryFileCount deliberately share one name, as they protect the same counter.
magnesj
marked this pull request as ready for review
August 24, 2026 12:47
kriben
approved these changes
Aug 24, 2026
… K filter Move the call to cellCornerVertices below the K filter early out, so the corner coordinates are only computed for the cells that are actually tested against the polygon.
magnesj
force-pushed
the
vizfwk-remove-orphaned-omp-pragmas
branch
from
August 24, 2026 13:38
3f23cc0 to
fa6e257
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14596
An orphaned
#pragma omp for(no enclosingomp parallel) is not compiled away by MSVC. It emits avcomp_barriersized for the calling thread's OpenMP team, so the thread blocks forever waiting for pool workers that never reach it. Incvfqt::Utils::toTextureImageRegion, reached fromcaf::Viewer::paintGL, this freezes the GUI.The fix removes the orphaned directives, and the rest of the branch is the follow up from reviewing all OpenMP use in the code base.
#pragma omp forcvfqt::Utils::toTextureImageRegion#pragma omp parallel forRifRoffFileToolsandRimCornerPointCase, given the parallelism it was meant to haveRigCellFaceGeometryTools: the reduction variable was never incremented, so thereservewas a no-op called unsynchronized by every threadorderedclauseRivNNCGeometryGenerator:orderedwith noorderedregion, plus a lock per triangle, replaced by per thread buffersRiaCurveMerger: each iteration writes to its own elementRimPolygonFilter: eight critical sections removed from per cell loopsdocs/agents/coding-style.mdEach commit is independent and can be dropped on its own. The parallelism change to the two import loops is a performance change, not a correctness fix, and has not been benchmarked.