Remove busy wait completely in TaskScheduler #2573
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2573 +/- ##
==========================================
- Coverage 93.16% 93.16% -0.01%
==========================================
Files 1027 1027
Lines 38418 38416 -2
==========================================
- Hits 35791 35789 -2
Misses 2627 2627 ☔ View full report in Codecov by Sentry. |
src/common/task_system/task.cpp
Outdated
| @@ -22,6 +22,10 @@ void Task::deRegisterThreadAndFinalizeTaskIfNecessary() { | |||
| finalizeIfNecessary(); | |||
| } catch (std::exception& e) { setExceptionNoLock(std::current_exception()); } | |||
| } | |||
| if (isCompletedNoLock()) { | |||
| mtx.unlock(); | |||
There was a problem hiding this comment.
wait shouldn't this be lck.unlock().
There was a problem hiding this comment.
Via #2574, seems like this is the issue.
src/include/main/client_context.h
Outdated
| inline uint64_t getTimeoutRemainingInMS() { | ||
| if (!isTimeOutEnabled()) { | ||
| return UINT64_MAX; | ||
| } |
There was a problem hiding this comment.
We could change this into a KU_ASSERT()... not sure if that makes more sense. I.e., make it an error to call this method when a timeout is not set.
| if (timedWait) { | ||
| task->cv.wait_for(taskLck, std::chrono::milliseconds(timeout)); | ||
| } else { | ||
| task->cv.wait(taskLck); | ||
| } | ||
| taskLck.unlock(); |
There was a problem hiding this comment.
One thing we should note in the code as a comment is that completed tasks may sit around in the queue for a bit.
|
I'm pretty sure we actually didn't fix the busy wait. In Basically, the issue is that we don't remove tasks from the queue once they have been scheduled on enough threads, I think. |
No description provided.