avoid long tail tasks due to PrioritySemaphore #11574
Conversation
Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
sql-plugin/src/main/scala/com/nvidia/spark/rapids/PrioritySemaphore.scala
Outdated
Show resolved
Hide resolved
| new PriorityQueue[ThreadInfo](Ordering.by[ThreadInfo, T](_.priority).reverse) | ||
| new PriorityQueue[ThreadInfo]( | ||
| // use task id as tie breaker when priorities are equal (both are 0 because never hold lock) | ||
| Ordering.by[ThreadInfo, T](_.priority).reverse. |
There was a problem hiding this comment.
nit: could we write this as
Ordering.by[ThreadInfo, T](t => (t.priority, t.taskId)).reverse
(technically this would flip the taskId comparison but I don't think we care)
There was a problem hiding this comment.
I can see the argument for wanting to be more explicit with thenComparing so that's totally fine too
There was a problem hiding this comment.
I think it matters ? We hope tasks with smaller taskid could have higher priority, so that we can avoid the very long tasks spanning from the start of stage to end of stage.
|
build |
Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
binmahone
force-pushed
the
241009_avoid_semaphore_long_tail
branch
from
2bebd2b
to
9956fd9
Compare
|
build |
| // use task id as tie breaker when priorities are equal (both are 0 because never hold lock) | ||
| Ordering.by[ThreadInfo, T](_.priority).reverse. | ||
| thenComparing((a, b) => a.taskId.compareTo(b.taskId)) | ||
| ) | ||
|
|
||
| def tryAcquire(numPermits: Int, priority: T): Boolean = { |
There was a problem hiding this comment.
This also needs a taskAttemptId and updates to the ordering comparison below otherwise the algorithm we're using for tryAcquire doesn't match the algorithm being used for waiting queue ordering (although it's very close). For example, a task with priority 0 and task attempt ID 2 with 5 permits will block a task with priority 0 and task attempt ID 1 with 2 permits, even if the semaphore had 4 permits available.
There was a problem hiding this comment.
You're right, it's a very corner case I did't pay attention to. Since your comment is after my merge action, I have submitted another PR to fix this: https://github.com/NVIDIA/spark-rapids/pull/11587/files. BTW, Is there any real cases that we'll have different permits for different threads?
There was a problem hiding this comment.
Is there any real cases that we'll have different permits for different threads?
Yes, because the concurrent GPU tasks config can be updated at runtime, and that changes the number of permits for subsequent tasks. See GpuSemaphore.computeNumPermits.
There was a problem hiding this comment.
I didn't realize "concurrent GPU tasks config can be updated at runtime", thanks !
* use task id as tie breaker Signed-off-by: Hongbin Ma (Mahone) <[email protected]> * save threadlocal lookup Signed-off-by: Hongbin Ma (Mahone) <[email protected]> --------- Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
* avoid long tail tasks due to PrioritySemaphore (#11574) * use task id as tie breaker Signed-off-by: Hongbin Ma (Mahone) <[email protected]> * save threadlocal lookup Signed-off-by: Hongbin Ma (Mahone) <[email protected]> --------- Signed-off-by: Hongbin Ma (Mahone) <[email protected]> * addressing jason's comment Signed-off-by: Hongbin Ma (Mahone) <[email protected]> --------- Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
sameerz
added
bug
This PR fixes #11573 by adding a tie breaker using task id.
Long tail tasks disappeared after the fix: