Multithreading Implementation #45

Sir-NoChill · 2024-09-08T13:49:52Z

Implemented Multi-Threaded test running 🧵

I dis-aggregated the test running and the result printing, spawning threads to execute each sub-package worth of tests independently. The number of threads is limited by a command line option.

Current Rough Edges

Termination Behavior

There is a long delay after the termination of the testing and the threads being cleaned up, though I think that could be fixed with the implementation I propose in the relevant commit.

Timing Accuracy

I'm not sure how much accuracy of timed tests will be affected by the threaded implementation. This is something to test, and in the worst case (that it does affect the timing a lot), we should repeat the timings with warm up and timed runs until we get a reasonable amount of data to average. This would be simple to implement, but I don't want to do so right now.

Load Balancing

The load balancing might suck a little bit here to begin with, but I propose a better architecture in my commit that implements the threading behavior. I did not implement it because it's going to be weird enough that I will need help from someone who actually knows how to write C++.

JustinMeimar · 2024-09-08T15:01:07Z

This looks great, I think we should leave it in this branch for now and test it out when marking generator.

Did you try testing the Gazprea solution with multi-threading? I'm curious to see how fast it was.

Nice work!

TestSet now contains data on the success of the test in a pair

Put the data into the optional. This was surprisingly non-trivial I really hate C++

Simply spawns a thread for every toolchain. Note that this would probably brick a computer without a lot of cores or running tests with a whole bunch of packages. Implementation will need to limit the number of spawnable threads in the future.

Currently there is a delay when we try to kill the threads, I'm not entirely sure why, but it makes it seem like the process takes a long time to die. Something to investigate in the future, but threading is fully implemented.

**Batch Size** Not necessary since the threads will just grab everything in their subpackage. In the future it would be nice to, instead of seperating based on packages: 1. Generate the set of all tests 2. Threads access batchSize tests at a time 3. Threads then keep referring back to the test pool for which tests need executing This would eliminate the need for constantly restarting threads, but would take a bit of a refactor to implement correctly. **Strict** I'm tired. I hate writing C++. I'm going to sleep. Here's my PR.

Forgot to move it from the printing function to the execution one.

Previously, the threads were individually executing an entire toolchain on their own, switched that to actually do the subpackage like I claimed it did.

Ensured that the grader does not try to execute everything in a single thread, waiting for the tests to finish asynchronously.

In preparation for the use of the set generation tactic I mentioned for better load balancing in the children

kuzi117 · 2024-09-10T15:04:35Z

include/testharness/TestHarness.h

@@ -54,14 +56,21 @@ class TestHarness {

 private:
  // The results of the tests.
+  //  NOTE we keep both a result manager and
+  //  the result in the TestSet to ensure in-ordre


ordre -> order

kuzi117

Nice work, this is a big endeavor, I just want to make sure everything is thread safe here and a couple minor comments on C++ references.

Will comment on the multithreading architecture in the main thread.

kuzi117 · 2024-09-10T15:08:23Z

include/testharness/TestHarness.h

  // test running
-  bool runTestsForToolChain(std::string tcId, std::string exeName);
+  void threadRunTestsForToolChain(std::string tcId, std::string exeName, std::reference_wrapper<SubPackage> subpackage);


Why is this std::reference_wrapper and not just SubPackage&? Typical use for std::reference_wrapper is for storage in containers or template shenanigans.

kuzi117 · 2024-09-10T15:12:10Z

src/testharness/TestHarness.cpp

+          }
+
+          // spawn a new thread executing its tests
+          std::thread t(&TestHarness::threadRunTestsForToolChain, this, tcPair.first, exePair.first, std::ref(subpackage.second));


You can just use subpackage.second if you convert the parameter to SubPackage&.

kuzi117 · 2024-09-10T15:17:30Z

include/testharness/TestHarness.h

@@ -17,7 +18,8 @@ namespace fs = std::filesystem;
 namespace tester {

 // Test hierarchy types
-typedef std::vector<std::unique_ptr<TestFile>> SubPackage;
+typedef std::pair<std::unique_ptr<TestFile>, std::optional<TestResult>> TestPair;


I realize these feel like much the same thing, but this shouldn't be std::option, it should be using the promise/future mechanism(std::promise, std::future). It has the same sort of mechanism as std::option for being in a limbo state with no value, but the outward appearance is immediately apparent to those familiar with the terms. "Option" should be for a value that may or may not have a value, "promise" is for a promised value that will arrive, just in the future, "future" is for getting that promised value. std::future also comes with the busy wait mechanism built in at std::future::wait.

In particular, std::optional should be replaced here with std::promise.

kuzi117 · 2024-09-10T15:22:20Z

src/analysis/Grader.cpp

+            // Poll while we wait for the result
+            // TODO this could probably be replaced with some sort of interrupt,
+            //  (and probably should be), but better this than no threads
+            while (!testpair.second.has_value())


std::future::wait here, or std::future::wait_for if you want a time out and give up, though the tester should be guaranteed to return since it has its own timeout.

Technically, I think option is probably not thread safe, so std::optional::has_value might return true before the value is set. I believe std::future is purpose-built to handle this.

kuzi117 · 2024-09-10T15:25:08Z

include/tests/TestResult.h


+  void swap(TestResult &other) {


Is there any reason that this can't just be std::swap on two instances of TestResult to begin with?

kuzi117 · 2024-09-10T15:26:16Z

include/tests/TestResult.h

+  bool error;
+  std::string diff;
+
+  TestResult clone() { return TestResult(this->name, this->pass, this->error, this->diff); }


This should be a copy constructor in TestResult, namely TestResult(const TestResult &other).

Looking further, I'm not sure this is ever necessary. But if it were, it should be the copy constructor or possibly operator=(const TestResult &other).

kuzi117 · 2024-09-10T15:32:29Z

src/testharness/TestHarness.cpp

+
+          // keep the result with the test for pretty printing
+          std::optional<TestResult> res_clone = std::make_optional(result.clone());
+          subPackage[i].second.swap(res_clone);


These three lines don't do anything.

pair is subPackage[i]. result is a duplicate of subPackage[i].second.value. res_clone is a an optional of subPackage[i].second.value. Then you swap subPackage[i].second with that value, which results in no change.

Can you explain what the intent is here?

kuzi117 · 2024-09-10T15:42:21Z

src/testharness/TestHarness.cpp

+
+      TestResult result = runTest(test.get(), toolChain, cfg);
+      // keep the result with the test for pretty printing
+      std::optional<TestResult> res_clone = std::make_optional(result.clone());


There's no need to clone here, since make_optional is just doing the same thing under the hood. std::optional doesn't store a reference (unless you're asking it to), so std::make_optional is going to call TestResult(const TestResult &) as part of initializing the stored value anyways.

In any case, if this is changed to promise/future, this line isn't needed anyways, since the next line should just change to using std::promise::set_value. No need to clone again.

Sir-NoChill · 2024-09-10T15:55:27Z

I should note that I am currently working on the load balancing fix I mentioned in the summary, though that might be best as a seperate PR anyways

kuzi117 · 2024-09-10T16:13:33Z

The way I see it, you have two options, one which I would lean on since you already have the mechanism set up.

I wouldn't pick this way, but it's here for completeness and you might like it. You can use std::async which returns a future immediately (the promise is managed internally) that you can wait on at a later time. Passing std::launch::async will have it put on another thread. Passing std::launch::deferred will just lazily evaluate (almost certainly don't want this). You can put them together since it's a bit mask, but it would be bad to defer it to later. This option is slightly annoying since you'll need to rework your evaluation mechanism to be function based, probably a free function, but it does free you from having to manually spawn and join threads. Unfortunately, spawning a bunch of threads this way is slow (you might not care) and it's not guaranteed there's a thread pool underlying it since it's compiler defined implementation behavior.
... but since you already have a thread pool, you might as well do that. My approach would be to have a work queue and a mutex lock. Fill the queue with work (packages to process) then spawn your threads. Threads start by trying to acquire a mutex (std::mutex::lock will block until it's available), on success they pop an item from the queue then release the mutex for another thread to allow it to grab work. They chug through the work, fill the promises, then go back to the mutex to get more work. If they get to the queue and it's empty, they quit.

I think you're already 75% of the way there with the implementation that's currently here. Just a bit of rearranging and some extra variables and you're good to go. Let me know if you have any questions.

kuzi117 · 2024-09-10T16:20:15Z

if you want to chat with me directly/more quickly about anything I put up above, you can email me at my ualberta email and I'll pass on my discord info or we can work something else out. Don't want to paste it here for fear of spam scrapers. It's braedy[at]ualberta.

Had an indexing error due to a malformed ternary, all works now

Now just to make the actual printing happen in the proper sequence

kuzi117 · 2024-09-17T01:38:36Z

src/testharness/TestHarness.cpp

+  }
+}
+
+void TestHarness::threadRunTestsForToolChain(std::reference_wrapper<std::vector<std::string>> tcNames,


I do believe that instead of std::reference_wrapper here, they should be just std::vector<XXX>&. Could be const as well.

kuzi117 · 2024-09-17T01:40:12Z

src/testharness/TestHarness.cpp

+
+void TestHarness::threadRunTestsForToolChain(std::reference_wrapper<std::vector<std::string>> tcNames,
+                                             std::reference_wrapper<std::vector<std::string>> exeNames,
+                                             std::reference_wrapper<std::vector<std::reference_wrapper<TestPair>>> tests,


The internal one here is correct. It should be std::vector<std::reference_wrapper<TestPair>>&.

kuzi117 · 2024-09-17T01:44:50Z

Nice work, that update looks like it should be really close to the final version. Just update those reference_wrapper uses and I'll take a look after. You didn't switch to using promises, but I think as long as you spawn threads, do work, and join before any other part of the main thread gets to examine the results, there should be no race conditions.

Sir-NoChill requested review from JustinMeimar and rcunrau September 8, 2024 13:49

Sir-NoChill force-pushed the ayrton-multithreading branch from d669016 to 92db975 Compare September 8, 2024 13:51

Sir-NoChill added 13 commits September 8, 2024 19:21

Added multithreading options

9ed543b

Refactored the TestSet

ab0ac82

TestSet now contains data on the success of the test in a pair

Populate result information in TestPair

e6777c0

Put the data into the optional. This was surprisingly non-trivial I really hate C++

Seperation of test running and result printing

71f9962

Naive multithreading support

c57ed72

Simply spawns a thread for every toolchain. Note that this would probably brick a computer without a lot of cores or running tests with a whole bunch of packages. Implementation will need to limit the number of spawnable threads in the future.

Remove unused function

1a65cc6

Implemented numThread limiting

e1ef0a1

Currently there is a delay when we try to kill the threads, I'm not entirely sure why, but it makes it seem like the process takes a long time to die. Something to investigate in the future, but threading is fully implemented.

Fixed comparison warning

723a411

Fixed Runtimes

f518f6c

Forgot to move it from the printing function to the execution one.

Fixed granularity of multithreading

66e52d1

Previously, the threads were individually executing an entire toolchain on their own, switched that to actually do the subpackage like I claimed it did.

Grader parity with TestHarness

6594a08

Ensured that the grader does not try to execute everything in a single thread, waiting for the tests to finish asynchronously.

Re added the batch size argument

2e44d29

In preparation for the use of the set generation tactic I mentioned for better load balancing in the children

Sir-NoChill force-pushed the ayrton-multithreading branch from 0e0a4aa to 2e44d29 Compare September 9, 2024 01:22

kuzi117 reviewed Sep 10, 2024

View reviewed changes

kuzi117 requested changes Sep 10, 2024

View reviewed changes

Sir-NoChill added 2 commits September 12, 2024 22:32

Multithreading works as expected

3afde6a

Had an indexing error due to a malformed ternary, all works now

Removed MT Debug info

ccf8cfd

Now just to make the actual printing happen in the proper sequence

kuzi117 reviewed Sep 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multithreading Implementation #45

Multithreading Implementation #45

Sir-NoChill commented Sep 8, 2024

JustinMeimar commented Sep 8, 2024

kuzi117 Sep 10, 2024

kuzi117 left a comment

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

kuzi117 Sep 10, 2024

Sir-NoChill commented Sep 10, 2024

kuzi117 commented Sep 10, 2024

kuzi117 commented Sep 10, 2024

kuzi117 Sep 17, 2024

kuzi117 Sep 17, 2024

kuzi117 commented Sep 17, 2024

Multithreading Implementation #45

Are you sure you want to change the base?

Multithreading Implementation #45

Conversation

Sir-NoChill commented Sep 8, 2024

Implemented Multi-Threaded test running 🧵

Current Rough Edges

Termination Behavior

Timing Accuracy

Load Balancing

JustinMeimar commented Sep 8, 2024

Choose a reason for hiding this comment

kuzi117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sir-NoChill commented Sep 10, 2024

kuzi117 commented Sep 10, 2024

kuzi117 commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuzi117 commented Sep 17, 2024