From: Simon Marchi Date: Fri, 19 Sep 2025 20:26:59 +0000 (-0400) Subject: gdbsupport: re-work parallel_for_each test, again X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8c53c1d9c4bf6787d4e12f90d73165863f6f726e;p=thirdparty%2Fbinutils-gdb.git 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::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 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 --- diff --git a/gdb/unittests/parallel-for-selftests.c b/gdb/unittests/parallel-for-selftests.c index f5456141ff6..559446deef4 100644 --- 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; -using do_foreach_t = gdb::function_view; +using do_foreach_t = gdb::function_view; +/* 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 input; - { - constexpr int upper_bound = 1000; - std::atomic 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 counter (0); - do_foreach (0, 0, [&] (int start, int end) { counter += end - start; }); - SELF_CHECK (counter == 0); - } + std::vector 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> intresults; - std::atomic any_empty_tasks (false); - - do_foreach (0, 1, - [&] (int start, int end) - { - if (start == end) - any_empty_tasks = true; - - return std::make_unique (end - start); - }); - - SELF_CHECK (!any_empty_tasks); - SELF_CHECK (std::all_of (intresults.begin (), intresults.end (), - [] (const std::unique_ptr &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 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 b74c8068cf2..50406763ded 100644 --- 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 void sequential_for_each (RandomIt first, RandomIt last, RangeFunction callback) { - callback (first, last); + if (first != last) + callback (first, last); } }