New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use WaitingTaskHolder for function arguments #32510
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32510/20448
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
This is in preparation for removing use of TBB deprecated tbb::task API.
44ccbc0
to
d1bc632
Compare
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32510/20451 ERROR: Build errors found during clang-tidy run.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32510/20455
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: DataFormats/Provenance @makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
FWCore/Framework/src/DataProxy.cc
Outdated
@@ -102,7 +103,7 @@ namespace edm { | |||
auto waitTaskPtr = waitTask.get(); | |||
auto token = ServiceRegistry::instance().presentToken(); | |||
edm::esTaskArena().execute([this, waitTaskPtr, &iRecord, &iKey, iEventSetupImpl, token]() { | |||
prefetchAsync(waitTaskPtr, iRecord, iKey, iEventSetupImpl, token); | |||
{ prefetchAsync(WaitingTaskHolder(waitTaskPtr), iRecord, iKey, iEventSetupImpl, token); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra braces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in an earlier version of my edit I had a WaitingTaskHolder variable so the scope was needed. Now they are just a left over.
FWCore/Framework/src/Worker.h
Outdated
prefetchAsync<T>(ownRunTask->release(), token, parentContext, info, T::transition_); | ||
[ownRunTask, parentContext, info = transitionInfo, token, this](std::exception_ptr const* iExcept) mutable { | ||
//NOTE: a non null value of iExcept means the selection succeeded | ||
if (!iExcept) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate? Was there a bug before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is part of a change I tried to back out. Evidently I missed a piece. I had decided to not touch this code as it is the only case where we pass in a task that isn't always meant to be run. So using a WaitingTaskHolder is not the right semantics for the function call.
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8b378a/11767/summary.html Comparison SummarySummary:
|
} | ||
} | ||
} | ||
waitTask->decrement_ref_count(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How useful is the
waitTask->set_ref_count(2);
{ ... }
waitTask->decrement_ref_count();
waitTask->wait_for_all();
compared to
waitTask->set_ref_count(1);
{ ... }
waitTask->wait_for_all();
when WaitingTaskHolder
constructor increases the reference count of the task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a left over from the conversion and isn't needed anymore. On the other hand, it doesn't really hurt either and will be completely replaced when we do the conversion to tbb::task_group
.
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
see #32563 |
} | ||
}); | ||
auto task = make_waiting_task(tbb::task::allocate_root(), | ||
[holder = std::move(iTask)](std::exception_ptr const* iExcept) mutable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bug, holder needs to be of type WaitingTaskForArenaHolder
PR description:
PR validation:
The code compiles and all framework related unit tests pass.