Add --enable-openmp to exercise ctlgeom thread-safety in test suite#79
Merged
Conversation
Adds m4/ax_openmp.m4 (autoconf-archive AX_OPENMP) and a --enable-openmp configure flag that appends OPENMP_CFLAGS to CFLAGS/LDFLAGS. geomtst.c and test-prism.c are wrapped with #pragma omp parallel for on the random-point/random-segment test loops so the ctlgeom entry points they exercise are hammered from multiple threads concurrently. random_object_and_lattice() in geomtst.c now takes a perturb_lattice flag; the parallel loops call it with 0 so they don't race on the global geometry_lattice (perturbed once in serial up-front instead). test-prism's file writes are guarded with #pragma omp critical, and counters use reduction(+:...). Default ./configure leaves OpenMP off and make check is unchanged. With --enable-openmp, make check will surface races in ctlgeom (notably reinit_prism via geom_fix_object_ptr).
Collaborator
|
I assume the OMP tests currently fail due to the lack of thread safety? |
Contributor
Author
|
Yes. With |
Collaborator
|
Okay, I'll merge this now, and when we have a patch that fixes the thread safety we can enable the tests during CI in that PR. |
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.
Closes #78.
Summary
Adds an opt-in
--enable-openmpconfigure flag and wraps the random-point / random-segment loops inutils/geomtst.candutils/test-prism.cwith#pragma omp parallel for, so the ctlgeom entry points they exercise are hit from multiple threads concurrently. The point is to give the test suite teeth against thread-safety regressions in libctlgeom.Changes
m4/ax_openmp.m4: adds the standardAX_OPENMPmacro from the autoconf-archive.configure.ac: new--enable-openmpflag (default off). When enabled, runsAX_OPENMPand appendsOPENMP_CFLAGStoCFLAGS/LDFLAGS.utils/geomtst.c:random_object_and_lattice()now takes anint perturb_latticeflag (per the issue, so it doesn't race on the globalgeometry_latticewhen called from threads).main()perturbs the lattice once in serial under#ifdef _OPENMPand runs the two test loops with#pragma omp parallel for.utils/test-prism.c:#pragma omp parallel for ... reduction(+:...)on the three random-input loops (test_point_inclusion,test_normal_to_object,test_line_segment_intersection); file-log writes guarded with#pragma omp critical;getenvhoisted out of the inner loop.No changes to
utils/geom.corutils/geom.scm. Actual thread-safety fixes belong in their own PR (see #75 and follow-ups).