Skip to content

test: add threadsafe execute test and sanitizer CI modes#832

Merged
DiamonDinoia merged 1 commit intoflatironinstitute:masterfrom
DiamonDinoia:feat/threadsafe-execute-tests
Mar 16, 2026
Merged

test: add threadsafe execute test and sanitizer CI modes#832
DiamonDinoia merged 1 commit intoflatironinstitute:masterfrom
DiamonDinoia:feat/threadsafe-execute-tests

Conversation

@DiamonDinoia
Copy link
Collaborator

Add threadsafe_execute regression test verifying concurrent execute() calls on the same plan produce correct results. Add sanitizer mode selection via FINUFFT_USE_SANITIZERS=OFF|ON|MEMSAN|TSAN, and extend the sanitizer GitHub workflow to run a focused Linux TSAN job.

@DiamonDinoia DiamonDinoia requested a review from lu1and10 March 16, 2026 18:47
@DiamonDinoia
Copy link
Collaborator Author

Hi @lu1and10,

This should be quick to review. Then I could clean up #831.

@DiamonDinoia DiamonDinoia force-pushed the feat/threadsafe-execute-tests branch from 6c71103 to d786291 Compare March 16, 2026 18:49
Add threadsafe_execute regression test verifying concurrent execute()
calls on the same plan produce correct results. Add sanitizer mode
selection via FINUFFT_USE_SANITIZERS=OFF|ON|MEMSAN|TSAN, and extend
the sanitizer GitHub workflow to run a focused Linux TSAN job.
@DiamonDinoia DiamonDinoia force-pushed the feat/threadsafe-execute-tests branch from d786291 to c1e4918 Compare March 16, 2026 18:51
Copy link
Member

@lu1and10 lu1and10 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me. More tests are good. I guess the Jenkins failure is not related.

std::vector<std::thread> workers;
workers.reserve(nthreads);
for (int tid = 0; tid < nthreads; ++tid) {
workers.emplace_back([&, tid]() {
Copy link
Member

Choose a reason for hiding this comment

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

I never tried this. Is this emplace_back non-block, so workers.reserve(nthreads) for nthreads==4 will execute 4 finufft_execute simultaneously, and that's how you test the thread safety of executing parallel execution?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A brief explanation here may be useful. workers.emplace_back(...) constructs each std::thread directly in the vector. The new thread begins running the lambda immediately, and the thread constructor returns without waiting for completion, so the loop launches all threads without blocking.

After that, I call join() on each thread to wait for them all to finish. So yes, for nthreads == 4, this is intended to execute 4 finufft_execute calls concurrently and test thread safety.

@DiamonDinoia DiamonDinoia merged commit d360f7f into flatironinstitute:master Mar 16, 2026
26 of 27 checks passed
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