Skip to content

Fix exec::thread_pool_base customization of bulk - #2226

Open
Cra3z wants to merge 4 commits into
NVIDIA:mainfrom
Cra3z:main
Open

Fix exec::thread_pool_base customization of bulk#2226
Cra3z wants to merge 4 commits into
NVIDIA:mainfrom
Cra3z:main

Conversation

@Cra3z

@Cra3z Cra3z commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Fix the missing customization of bulk_unchunked in exec::thread_pool_base. Additionally, the current bulk customization in exec::thread_pool_base executes in parallel regardless of the execution policy, which is incorrect. It should only execute in parallel across multiple execution agents when the policy is std::execution::par or std::execution::par_unseq; otherwise, it should fall back to sequential execution on a single execution agent.

fixes #1687

…rrectly handle non-parallel execution policies
@copy-pr-bot

copy-pr-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ericniebler ericniebler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks!

fixes #1687

@ericniebler

Copy link
Copy Markdown
Collaborator

/ok to test 7214721

@ericniebler

Copy link
Copy Markdown
Collaborator
The following tests FAILED:
	818 - exec::bwos::lifo_queue - high contention stress (Timeout)

@Cra3z, looks like you need to bump up the timeout for this test.

@Cra3z

Cra3z commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@ericniebler Uh... odd, I've never touched the lifo_queue code. On my local MSVC 14.51, the exec::bwos::lifo_queue - high contention stress test case doesn't seem to time out.

# Conflicts:
#	include/exec/thread_pool_base.hpp
@Cra3z

Cra3z commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@ericniebler I dug into this one — it does reproduce, but I don't think raising the
timeout is the right fix: the test's runtime is a function of how many cores the
runner happens to give us, so any fixed timeout is just a guess.

Reproduction

Pinning ctest to a single core (plus a few competing spinners, to emulate a
contended runner vCPU) reproduces the CI failure almost exactly:

local:  1/1    Test #292: exec::bwos::lifo_queue - high contention stress ...***Timeout  32.10 sec   (ctest exit=8)
CI:     818/1022 Test #818: exec::bwos::lifo_queue - high contention stress ...***Timeout  32.31 sec   (ctest: 8)

Root cause

The owner's push loop yields, but the thief loop has no backoff at all:
owner, line 357 — yields:

std::thread owner(
[&]()
{
for (std::size_t i = 1; i <= numItems; ++i)
{
while (!queue.push_back(i))
{
std::this_thread::yield();
}
pushCount++;
}
done = true;
});

thief, line 376 — pure busy-spin:

thieves.emplace_back(
[&, t]()
{
std::size_t localCount = 0;
while (true)
{
auto val = queue.steal_front();
if (val != 0)
{
localCount++;
}
else if (done)
{
break;
}
}
thiefCounts[t] = localCount;
});

With 8 thieves burning full quanta on a mostly-failing steal_front(), the owner
gets roughly 1/9 of the scheduler's share. Throughput collapses as cores are removed:

cores wall time
16 0.02 s
4 (CI runner) 2.5 s
2 10.8 s
1 25.5 s

A watchdog confirms pushCount climbs linearly the entire time, so this is
throughput collapse rather than a livelock or a deadlock. Two things I ruled out:
ASan is not the driver (0.010 s → 0.018 s at 16 cores), and neither is
msvc 14.50 vs 14.51. The 4-vCPU runner alone only gets to ~2.5 s — the rest
appears to come from the Docker --isolation=process overhead and a contended
shared vCPU, which is why it's flaky rather than consistently failing.

Possible fix

Give the thief loop the same backoff the push loop already has:

           {
             break;
           }
+          else
+          {
+            std::this_thread::yield();
+          }
         }
         thiefCounts[t] = localCount;
       });

@Cra3z
Cra3z requested a review from ericniebler August 31, 2026 12:47
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.

thread pools based on execpools::thread_pool_base are not customizing the bulk* algorithms correctly

2 participants