Use WaitingTaskHolder to signal doneWaiting() instead of WaitingTaskWithArenaHolder in framework
#47029
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47029/43145
|
|
A new Pull Request was created by @makortel for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
And I forgot the EventSetup side |
| ModuleContextSentry moduleContextSentry(&moduleCallingContext_, parentContext); | ||
| try { | ||
| convertException::wrap([&]() { this->implDoAcquire(info, &moduleCallingContext_, holder); }); | ||
| WaitingTaskWithArenaHolder holderWithArena{holder}; | ||
| convertException::wrap([&]() { this->implDoAcquire(info, &moduleCallingContext_, holderWithArena); }); |
There was a problem hiding this comment.
The runAcquire() is called by runAcquireAfterAsyncPrefetch() that owns the holder beyond the scope of runAcquire() function. Therefore the destructor or doneWaiting() of this holderWithArena should not lead to the task reference count to drop to 0.
It could be cleaner to percolate the holder as WaitingTaskHolder through the implDoAcquire() functions, but that seemed a little bit tedious. I could do it nevertheless, but I wanted to get feedback on this PR in general first.
| @@ -42,7 +42,7 @@ namespace edm { | |||
|
|
|||
| // Takes ownership of the underlying task and uses the current | |||
| // arena. | |||
| explicit WaitingTaskWithArenaHolder(WaitingTaskHolder&& iTask); | |||
| explicit WaitingTaskWithArenaHolder(WaitingTaskHolder iTask); | |||
There was a problem hiding this comment.
This change allows a WaitingTaskHolder to be "implicitly copied" into WaitingTaskWithArenaHolder, i.e.
WaitingTaskHolder h;
WaitingTaskWithArenaHolder wta{h};
// instead of
WaitingTaskHolder h;
WaitingTaskWithArenaHolder wta{WaitingTaskHolder{h}};The pattern of WaitingTaskHolder being moved should work as before.
…ithArenalHolder in framework In the framework the doneWaiting() is always called from within the main arena in the TBB thread pool, and therefore using WaitingTaskHolder is safe (WaitingTaskWithArenaHolder is needed only when doneWaiting() is called outside of the TBB arena). Avoiding WaitingTaskWithArenaHolder allows to avoid enqueue() operation when the doneWaiting() calls in the framework are the ones that decrease the task reference count to 0.
makortel
force-pushed
the
waitingTaskWithArenaHolder
branch
from
1d9168d to
16a69d8
Compare
Now the EventSetup side should be included as well |
|
enable gpu, threading |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47029/43146
|
|
Pull request #47029 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
|
@cmsbuild, please test |
|
-1 Failed Tests: GpuUnitTests GPU Unit TestsI found 2 errors in the following unit tests: ---> test testCudaDeviceAdditionWrapper had ERRORS ---> test testCudaDeviceAdditionKernel had ERRORS Comparison SummarySummary:
GPU Comparison SummarySummary:
|
AFAICT these tests are independent of the changes in this PR |
|
@cmsbuild, please test |
|
-1 Failed Tests: GpuUnitTests GPU Unit TestsI found 2 errors in the following unit tests: ---> test testCudaDeviceAdditionKernel had ERRORS ---> test testCudaDeviceAdditionWrapper had ERRORS Comparison SummarySummary:
GPU Comparison SummarySummary:
|
|
Ah, the tests actually fail in the IBs (#46864) |
PR description:
In the framework the
doneWaiting()is always called from within the main arena in the TBB thread pool, and therefore usingWaitingTaskHolder is safe (
WaitingTaskWithArenaHolderis needed only whendoneWaiting()is called outside of the TBB arena).Avoiding
WaitingTaskWithArenaHolderallows to avoid enqueue() operation when thedoneWaiting()calls in the framework are the ones that decrease the task reference count to 0.Resolves cms-sw/framework-team#1125
PR validation:
Checked with gdb that a single-threaded test configuration with a module that uses
ExternalWork(Alpaka-based module on the CPU serial backend, to be exact) where theacquire()does not leave anyWaitingTaskWithArenaHolderalive (i.e. theacquire()does not really launch any asynchronous work) does not lead anymore to two TBB threads as a consequence of thetask_arena::enqueue().