Implement multi-threading in NativeRunner #91

jmaack24 · 2025-12-11T22:27:17Z

No description provided.

Copilot

Pull request overview

This PR implements multi-threading support in the NativeRunner to enable parallel ray tracing. The implementation introduces a ThreadManager class to coordinate multiple worker threads, refactors the TRayData structure to handle thread-safe ray data collection, and updates the trace functions to support concurrent execution.

Key changes include:

Addition of ThreadManager class for thread coordination and monitoring
Refactoring of ray tracing to split work across multiple threads with unique seeds
Conversion of ray numbering from unsigned int to uint_fast64_t to support larger simulations

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
thread_manager.hpp/cpp	New ThreadManager class for managing multiple worker threads, tracking progress, and handling cancellation
trace.hpp/cpp	Refactored trace_native to spawn multiple threads and added trace_single_thread for per-thread execution
native_runner.hpp/cpp	Added thread count and seed management, delegated status/cancel operations to ThreadManager
native_runner_types.hpp/cpp	Completely refactored TRayData from block-based storage to thread-indexed map structure for concurrent access
pt_optimizations.hpp/cpp	Changed rec_hash parameter from reference to pointer for thread sharing
process_interaction.cpp	Removed commented-out myrng_counter increments
generate_ray.hpp/cpp	Made PosSunStage parameter const for thread safety
simulation_runner.hpp	Added UNKNOWN status and status_string utility function
native_runner_test.cpp	Changed EXPECT_EQ to ASSERT_EQ in critical assertions, added tests for multi-threaded cancellation and ray ID assignment
native_runner_multithreading_test.cpp	New regression test for multi-threaded performance validation
count_absorbed_native.cpp	Updated ray variable type from unsigned int to uint_fast64_t
simulation_data.hpp	Enhanced comment documentation for clear() method
mtrand.hpp	Code formatting improvements (whitespace and brace placement)
CMakeLists.txt	Added thread_manager.cpp to build and new multithreading test file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coretrace/simulation_runner/native_runner/native_runner.cpp

coretrace/simulation_data/simulation_data.hpp

coretrace/simulation_runner/native_runner/trace.cpp

coretrace/simulation_runner/native_runner/native_runner.hpp

coretrace/simulation_runner/native_runner/native_runner_types.cpp

coretrace/simulation_runner/native_runner/native_runner.hpp

coretrace/simulation_runner/native_runner/thread_manager.cpp

coretrace/simulation_runner/native_runner/trace.cpp

Copilot

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coretrace/simulation_runner/native_runner/trace.cpp

coretrace/simulation_runner/native_runner/sun_to_primary_stage.cpp

coretrace/simulation_runner/native_runner/native_runner_types.hpp

coretrace/simulation_runner/native_runner/trace.cpp

google-tests/regression-tests/native_runner_multithreading_performance_test.cpp

coretrace/simulation_runner/native_runner/thread_manager.cpp

…ancel multithread test

…for debug builds

qualand

I merged this into my validation branch and see no performance change when switching to multi-threading. Maybe we should discuss when you get a chance.

qualand · 2025-12-16T22:56:10Z

coretrace/simulation_runner/native_runner/native_runner.cpp

+        // // TODO: This should throw an error...
+        // // Get RaySource data (this runner assumes there is only the Sun)
+        // assert(data->get_number_of_ray_sources() == 1);
+


qualand · 2025-12-16T22:56:21Z

coretrace/simulation_runner/native_runner/native_runner.cpp

+        if (data->get_number_of_elements() <= 0)
+        {
+            throw std::invalid_argument("SimulationData has no elements.");
+            // return RunnerStatus::ERROR;


qualand · 2025-12-16T23:15:02Z

coretrace/simulation_runner/native_runner/trace.cpp

+		ThreadInfo my_info;
+		my_info.manager = manager;
+		my_info.System = System;
+		my_info.NumberOfRays = NumberOfRays / nthreads;


This method does not preserve the expected ray count. Can we set unique number of rays by thread? This would allow each thread to run the truncated value of <NumberOfRays / nthreads> and one thread with the added remainder.

qualand · 2025-12-16T23:23:14Z

coretrace/simulation_runner/native_runner/trace.cpp

+		TSystem *System,
+		unsigned int seed,
+		uint_fast64_t NumberOfRays,
+		uint_fast64_t MaxNumberOfRays, // TODO: How to handle MaxRays?


We can handle the Max number of rays the same way as the number of rays. Each thread assigned with the truncated value of MaxNumberOfRays / nthreads, then one thread gets the remainder. This should preserve the user input of MaxNumberOfRays.

qualand · 2025-12-16T23:27:18Z

google-tests/regression-tests/native_runner_multithreading_performance_test.cpp

+
+TEST(NativeRunner, Multithreading)
+{
+    const uint_fast64_t NRAYS = 50000;


Can you add a check that confirms NRAYS in preserved in results?

qualand · 2025-12-16T23:30:34Z

google-tests/unit-tests/simulation_runner/native_runner/native_runner_multithreading_test.cpp

+        runner.print_log(std::cout);
+    }
+
+    ASSERT_EQ(nrec, NRAYS);


This works because 50,000 is divisible by 4.

qualand · 2025-12-16T23:36:55Z

google-tests/unit-tests/simulation_runner/native_runner/native_runner_test.cpp

+    std::cout << "Num Elements: " << sd.get_number_of_elements() << std::endl;
+
+    // Parameters
+    SimulationParameters &params = sd.get_simulation_parameters();


Is there a reason why should not put number of threads in the simulation parameters?

Number of threads is really a property of the ray tracer and not the ray tracing problem. It logically doesn't belong in the simulation parameters.

taylorbrown75 · 2025-12-18T22:17:49Z

I merged this into my validation branch and see no performance change when switching to multi-threading. Maybe we should discuss when you get a chance.

I tested this by merging it into the legacy GUI branch, and adding threads did not speed up the simulation in my case either.

qualand · 2025-12-18T23:20:18Z

I merged this into my validation branch and see no performance change when switching to multi-threading. Maybe we should discuss when you get a chance.

I tested this by merging it into the legacy GUI branch, and adding threads did not speed up the simulation in my case either.

Taylor, I resolved this issue on my end yesterday. The reason I didn't see a speed increase was because the number of rays I was running was too small and the ray data process method after the trace was the bigger bottleneck. Just an FYI.

qualand

Thanks for updating the method to maintain desired ray count!

taylorbrown75 · 2025-12-19T18:28:07Z

I merged this into my validation branch and see no performance change when switching to multi-threading. Maybe we should discuss when you get a chance.

I tested this by merging it into the legacy GUI branch, and adding threads did not speed up the simulation in my case either.

Taylor, I resolved this issue on my end yesterday. The reason I didn't see a speed increase was because the number of rays I was running was too small and the ray data process method after the trace was the bigger bottleneck. Just an FYI.

My issue is related to running in debug from the GUI. I ran the same stinput case via testing in release and saw speed improvement with added threads.

jmaack24 · 2025-12-19T18:29:44Z

Any objections to merging?

Initial implementation of multi-threading in NativeRunner

e070d60

jmaack24 requested a review from Copilot December 11, 2025 22:27

jmaack24 self-assigned this Dec 11, 2025

jmaack24 linked an issue Dec 11, 2025 that may be closed by this pull request

Implement NativeRunner multi-threading capability #90

Closed

Copilot started reviewing on behalf of jmaack24 December 11, 2025 22:27 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

jmaack24 added 2 commits December 15, 2025 10:07

Fix seeding issue; address copilot comments; implement logging

33f502b

Add missing header

6d69d92

jmaack24 requested a review from Copilot December 15, 2025 17:22

Copilot AI reviewed Dec 15, 2025

View reviewed changes

jmaack24 added 7 commits December 15, 2025 11:25

Run performance tests only for release build; adjust parameters for c…

d4cc833

…ancel multithread test

Fix clear and setup call ordering in RayData

4df9a1d

Update profile flags; fix printf compiler warning

1801914

Still trying to fix coverage

832b4c6

Remove multi-threading tests from coverage; remove performance tests …

253307d

…for debug builds

Add missing header

d2f36f0

Increase test coverage; address copilot comments

8dcc784

jmaack24 requested review from qualand and taylorbrown75 December 16, 2025 22:26

qualand requested changes Dec 16, 2025

View reviewed changes

jmaack24 added 2 commits December 17, 2025 12:27

Make number of rays traced consistent across number of threads

537c73c

Increase time for native runner validation test

716e358

qualand approved these changes Dec 19, 2025

View reviewed changes

qualand merged commit 51f7ecc into develop Dec 19, 2025
12 checks passed

qualand deleted the 90-implement-nativerunner-multi-threading-capability branch December 19, 2025 18:34

Implement multi-threading in NativeRunner #91

Implement multi-threading in NativeRunner #91

Uh oh!

Conversation

jmaack24 commented Dec 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qualand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylorbrown75 commented Dec 18, 2025

Uh oh!

qualand commented Dec 18, 2025

Uh oh!

qualand left a comment

Choose a reason for hiding this comment

Uh oh!

taylorbrown75 commented Dec 19, 2025

Uh oh!

jmaack24 commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qualand left a comment •

edited

Loading