gdbsupport: re-work parallel_for_each test, again

I started working on this patch because I noticed that this
parallel_for_each test:

    /* Check that if there are fewer tasks than threads, then we won't
       end up with a null result.  */

is not really checking anything.  And then, this patch ended with
several changes, leading to general refactor of the whole file.

This test verifies, using std::all_of, that no entry in the intresults
vector is nullptr.  However, nothing ever adds anything to intresults.
Since the vector is always empty, std::all_of is always true.  This
state probably dates back to afdd1366358c ("Back out some
parallel_for_each features"), which removed the ability for
parallel_for_each to return a vector of results.  That commit removed
some tests, but left this one in, I'm guessing as an oversight.

One good idea in this test is to check that the worker never receives
empty ranges.  I think we should always test for that.  I think it's
also a good idea to test with exactly one item, that's a good edge case.

To achieve this without adding some more code duplication, factor out
the core functionality of the test in yet another test_one function (I'm
running out of ideas for names).  In there, check that the range
received by the worker is not empty.  Doing this pointed out that the
worker is actually called with empty ranges in some cases, necessitating
some minor changes in parallel-for.h.

Then, instead of only checking that the sum of the ranges received by
worker functions is the right count, save the elements received as part
of those ranges (in a vector), and check that this vector contains each
expected element exactly once.  This should make the test a bit more
robust (otherwise we could have the right number of calls, but on the
wrong items).

Then, a subsequent patch in this series changes the interface or
parallel_for_each to use iterator_range.  The only hiccup is that it
doesn't really work if the "RandomIt" type of the parallel_for_each is
"int".  iterator_range<int>::size wouldn't work, as std::distance
doesn't work on two ints.  Fix this in the test right away by building
an std::vector<int> to use as input.

Finally, run the test with the default thread pool thread count in
addition to counts 0, 1 an 3, currently tested.  I'm thinking that it
doesn't hurt to test parallel_for_each in the configuration that it's
actually used with.

Change-Id: I5adf3d61e6ffe3bc249996660f0a34b281490d54
Approved-By: Tom Tromey <tom@tromey.com>

diff --git a/gdb/unittests/parallel-for-selftests.c b/gdb/unittests/parallel-for-selftests.c
index f5456141ff6ee41fe1ec8a5330cd75d4e2198359..559446deef43c91eb69bcc6a52b0439d463184b9 100644 (file)
--- a/gdb/unittests/parallel-for-selftests.c
+++ b/gdb/unittests/parallel-for-selftests.c
@@ -42,50 +42,56 @@ struct save_restore_n_threads
   int n_threads;
 };
 
-using foreach_callback_t = gdb::function_view<void (int first, int last)>;
-using do_foreach_t = gdb::function_view<void (int first, int last,
+using foreach_callback_t = gdb::function_view<void (int *first, int *last)>;
+using do_foreach_t = gdb::function_view<void (int *first, int *last,
                                             foreach_callback_t)>;
 
+/* Run one parallel-for-each test on the range [1, UPPER_BOUND) using the
+   parallel-for-each implementation DO_FOREACH.  */
+
 static void
-test_one (int n_threads, do_foreach_t do_foreach)
+test_one (do_foreach_t do_foreach, int upper_bound)
 {
-  save_restore_n_threads saver;
-  gdb::thread_pool::g_thread_pool->set_thread_count (n_threads);
+  std::vector<int> input;
 
-  {
-    constexpr int upper_bound = 1000;
-    std::atomic<int> counter (0);
-    do_foreach (0, upper_bound,
-               [&] (int start, int end) { counter += end - start; });
-    SELF_CHECK (counter == upper_bound);
-  }
+  for (int i = 0; i < upper_bound; ++i)
+    input.emplace_back (i);
 
-  {
-    std::atomic<int> counter (0);
-    do_foreach (0, 0, [&] (int start, int end) { counter += end - start; });
-    SELF_CHECK (counter == 0);
-  }
+  std::vector<int> output;
+  std::mutex mtx;
 
-  {
-    /* Check that if there are fewer tasks than threads, then we won't
-       end up with a null result.  */
-    std::vector<std::unique_ptr<int>> intresults;
-    std::atomic<bool> any_empty_tasks (false);
-
-    do_foreach (0, 1,
-               [&] (int start, int end)
-                 {
-                   if (start == end)
-                     any_empty_tasks = true;
-
-                   return std::make_unique<int> (end - start);
-                 });
-
-    SELF_CHECK (!any_empty_tasks);
-    SELF_CHECK (std::all_of (intresults.begin (), intresults.end (),
-                            [] (const std::unique_ptr<int> &entry)
-                              { return entry != nullptr; }));
-  }
+  do_foreach (input.data (), input.data () + input.size (),
+             [&] (int *start, int *end)
+               {
+                 /* We shouldn't receive empty ranges.  */
+                 SELF_CHECK (start != end);
+
+                 std::lock_guard lock (mtx);
+
+                 for (int *i = start; i < end; ++i)
+                   output.emplace_back (*i * 2);
+               });
+
+  /* Verify that each item was processed exactly once.  */
+  SELF_CHECK (output.size () == upper_bound);
+  std::sort (output.begin (), output.end ());
+
+  for (int i = 0; i < output.size (); ++i)
+    SELF_CHECK (output[i] == i * 2);
+}
+
+/* Run all tests on the parallel-for-each implementation DO_FOREACH.  */
+
+static void
+test_one_function (int n_threads, do_foreach_t do_foreach)
+{
+  save_restore_n_threads saver;
+  gdb::thread_pool::g_thread_pool->set_thread_count (n_threads);
+
+  /* Test with a few arbitrary number of items.  */
+  test_one (do_foreach, 0);
+  test_one (do_foreach, 1);
+  test_one (do_foreach, 1000);
 }
 
 static void
@@ -93,15 +99,17 @@ test_parallel_for_each ()
 {
   const std::vector<do_foreach_t> for_each_functions
     {
-      [] (int start, int end, foreach_callback_t callback)
+      [] (int *start, int *end, foreach_callback_t callback)
       { gdb::parallel_for_each<1> (start, end, callback); },
-      [] (int start, int end, foreach_callback_t callback)
+      [] (int *start, int *end, foreach_callback_t callback)
       { gdb::sequential_for_each (start, end, callback);}
     };
 
-  for (int n_threads : { 0, 1, 3 })
+  int default_thread_count = gdb::thread_pool::g_thread_pool->thread_count ();
+
+  for (int n_threads : { 0, 1, 3, default_thread_count })
     for (const auto &for_each_function : for_each_functions)
-      test_one (n_threads, for_each_function);
+      test_one_function (n_threads, for_each_function);
 }
 
 } /* namespace parallel_for */

diff --git a/gdbsupport/parallel-for.h b/gdbsupport/parallel-for.h
index b74c8068cf2c26b0b8d117dc45db780ce9094a96..50406763ded4eeadff80a3b362b7843d380fd798 100644 (file)
--- a/gdbsupport/parallel-for.h
+++ b/gdbsupport/parallel-for.h
@@ -128,7 +128,9 @@ parallel_for_each (RandomIt first, RandomIt last, RangeFunction callback)
                    (size_t)(last - first));
       debug_printf (_("\n"));
     }
-  callback (first, last);
+
+  if (first != last)
+    callback (first, last);
 
   for (auto &fut : results)
     fut.get ();
@@ -142,7 +144,8 @@ template<class RandomIt, class RangeFunction>
 void
 sequential_for_each (RandomIt first, RandomIt last, RangeFunction callback)
 {
-  callback (first, last);
+  if (first != last)
+    callback (first, last);
 }
 
 }