Sort incoming device tasks using heap

[devreal]

Currently, sorting of device tasks is broken (they don't actually contain a priority field) and linear over a slice of the input list. Instead, we can use a max-heap to keep a sorted array of device tasks, push all new tasks into the heap, and take out the highest priority task.

Complexity of inserting the ring into the heap is O(N+M) or O(N*logM), depending on the whether we just insert elements one by one or rebuild from scratch (beneficial if we insert more tasks than exist already).

We can detach the full lifo at once to avoid repeated atomic operations by the management thread.

Also, use nolock variants on stream-local lists and change to lifo for the shared queue to avoid the lock of the fifo. They are only modified by the device management thread. Inter-thread communication happens through the pending queue.

Once tasks are trickling through the system they do not need to be sorted anymore.

[Copilot]

Pull request overview

This PR reworks GPU device task intake to restore proper priority-based selection and reduce overhead by switching from FIFO-based pending queues and ad-hoc sorting to a lock-free LIFO intake plus a manager-private max-heap for prioritized pop.

Changes:

• Replace shared pending queues in CUDA and Level Zero devices with a lock-free parsec_lifo_t, and introduce a pending_heap (parsec_heap_*) used by the device management thread to pop the highest-priority task.
• Remove the old “sort pending tasks” mechanism and MCA parameters (sort_pending_tasks) along with the associated function pointer in parsec_device_module_t.
• Add parsec/class/parsec_heap.{h,c} and integrate it into the build, plus add parsec_lifo_detach_all() helpers to batch-move tasks with fewer atomic operations.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
parsec/mca/device/level_zero/device_level_zero_module.c	Switch Level Zero pending queue to LIFO and initialize/finalize the new pending heap.
parsec/mca/device/level_zero/device_level_zero_component.c	Remove the Level Zero MCA knob and wiring for pending-task sorting.
parsec/mca/device/device.h	Remove the sort_pending_list function pointer from the device module interface.
parsec/mca/device/device_gpu.h	Introduce pq_priority, swap pending type to LIFO, and add a manager-private pending heap field.
parsec/mca/device/device_gpu.c	Detach all pending tasks from LIFO, assign heap priorities, push into heap, and pop highest-priority task; switch stream-local queues to nolock list ops.
parsec/mca/device/cuda/device_cuda_module.c	Switch CUDA pending queue to LIFO and initialize/finalize the new pending heap.
parsec/mca/device/cuda/device_cuda_component.c	Remove the CUDA MCA knob and wiring for pending-task sorting.
parsec/CMakeLists.txt	Add class/parsec_heap.c to the build.
parsec/class/parsec_heap.h	Add new array-based max-heap API (pointer elements, int32_t priority key by offset).
parsec/class/parsec_heap.c	Implement heap push/pop and ring batch-push with adaptive heapify strategy.
parsec/class/lifo.h	Add helpers to detach the entire LIFO at once and convert the internal chain into a doubly-linked ring.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[devreal]

Looking into merging the two heap implementations. Didn't see the existing one because it wasn't in the class/ directory.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

[bosilca]

Overall a good addition, and a significant fix.

Few nitpicks:

• You remove spaces at the end of the lines. This is uncalled for in the context of another commit.
• I just realized that the stream fifo_pending is not ordered by priority anymore. Indeed, now that tasks can bypass the input stream (which would have guaranteed the original, ordered by priority, order), they will land in the execution streams unordered. I don't think this is a problem, but your last point in the PR description is incorrect.
• you mention O(N+M) cost, but I don't see where that would be. The current push_ring code is o(N log(N+M)).

[devreal]

I replaced the ring with a chain. To avoid iterating over it, we can set the priority when scheduling the device task. We only set the priority if the device task priority is negative (set in constructor), otherwise we take what the caller has set.

[devreal]

The complexity was a leftover from when I had this implemented using an array. I removed references to the complexity.

[bosilca]

The core heap preserves max-priority ordering in ordinary operation, and the current CI is green. I am requesting changes because the current head still has four blockers: paranoid builds lose or crash on detached task chains, equal-priority intake can indefinitely starve existing work, directly enqueued D2D-completion tasks receive the wrong priority, and the default development-header installation is broken. The inline comments include suggested fixes and targeted regression tests.

[bosilca]

P1 — this breaks PARSEC_DEBUG_PARANOID chain detachment. In the CAS128 implementation, the preceding loop advances item to NULL, so this call receives NULL and the function returns NULL after atomically removing every queued task. In the LLSC and lock-backed implementations, PARSEC_CHAIN_DETACH calls PARSEC_ITEM_DETACH on each element, which poisons list_next/list_prev with 0xdeadbeef; parsec_heap_push_chain() then attempts to traverse those poisoned links.

Please preserve the chain links until heap insertion has saved each next pointer, while still performing the paranoid ownership/refcount bookkeeping. The duplicate CAS128 detach loop should also be removed. A paranoid-build test that detaches and inserts a multi-item chain would catch both failure modes.

[bosilca]

P1 — equal-priority tasks can starve indefinitely. Equal elements neither sift up nor sift down. After a pop, the newest last leaf replaces the root and remains there because neither child is strictly greater. If one or more equal-priority tasks arrive before each subsequent pop, newly arrived work keeps cycling through the root while the older backlog remains below it indefinitely. Most device tasks use the same default priority, so this is a common arrival pattern, not just an edge case.

Please provide stable/FIFO ordering within a priority, for example with a monotonic enqueue sequence used as the secondary comparison key or per-priority FIFO queues. Add a test that continuously inserts and pops equal-priority elements while verifying that the initial backlog drains.

[bosilca]

P1 — this direct enqueue bypasses priority inheritance. The wrapper is constructed with gpu_task->priority == -1; only gpu_task->ec->priority is assigned INT32_MAX. The heap compares the wrapper field, and inheritance occurs only in parsec_device_kernel_scheduler(), which this path does not call. Consequently this supposedly urgent D2D-completion command enters at the lowest priority and can starve behind normal work, delaying source-reader release and GPU-memory reclamation.

Please set gpu_task->priority = INT32_MAX here, or centralize priority resolution in a common pending-enqueue helper used by all direct and scheduler enqueue paths. This needs a test covering the priority of directly enqueued D2D completion work.

[bosilca]

P1 — the default development-header installation is now unusable. parsec/maxheap.h is installed when PARSEC_WITH_DEVEL_HEADERS is enabled (the default), but the newly included parsec/class/parsec_heap.h is not present in any install list. External compilation including parsec/maxheap.h will therefore fail.

Please install the new dependency and add an install-tree compilation test. This header also replaces the externally exposed parsec_heap_t with parsec_task_heap_t; that source/API break should either be avoided (for example by giving the new generic heap a non-conflicting name) or handled as an intentional compatibility/version change.