Skip to content

Conversation

@btovar
Copy link
Member

@btovar btovar commented Nov 5, 2025

Use an array of priorities instead of a single priority per element. Priorities are compared in lexicographical order.

The idea is to be able to do:

        double priority = VINE_PRIORITY_DEFAULT;
	if (t->type == VINE_TASK_TYPE_RECOVERY) {
		priority = VINE_PRIORITY_RECOVERY;
	} else if (t->result == VINE_RESULT_RESOURCE_EXHAUSTION) {
		priority = VINE_PRIORITY_EXHAUSTION;
	}

	priority_queue_push_varargs(q->ready_tasks, t, priority, t->priority);

where all RECOVERY come before all EXHAUSTION, etc.

Merge Checklist

The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.

  • make test Run local tests prior to pushing.
  • make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
  • make lint Run lint on source code prior to pushing.
  • Manual Update: Update the manual to reflect user-visible changes.
  • Type Labels: Select a github label for the type: bugfix, enhancement, etc.
  • Product Labels: Select a github label for the product: TaskVine, Makeflow, etc.
  • PR RTM: Mark your PR as ready to merge.

@btovar btovar requested review from JinZhou5042 and dthain November 5, 2025 17:48
Copy link
Member

@dthain dthain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the elegance of this approach, allowing for priority vectors of arbitrary size. But I do wonder if the flexibility is worth the cost at the heart of the scheduler. Does this materially change the performance of the data structure? (due to extra mallocs). Would we be satisfied with simply having two values: a priority and a sub-priority?

@JinZhou5042
Copy link
Member

One concern I'm having is that the priority data structure is widely used in other scenarios, to which only one priority value would make sense. As multiple priority values only apply to task scheduling, would it be easier to store the two priority values in struct task and switch between them when needed?

@btovar
Copy link
Member Author

btovar commented Nov 5, 2025

With only one priority, the only thing that changes is the call to priority_queue_create.

I think we still want the option for more than two, and it is likely that we will find situations where we want to have more control in the way tasks are ordered. I'm going to change it so that the two (or maybe three) priority values are stored in the struct element, so that only one malloc per push is needed.

@btovar
Copy link
Member Author

btovar commented Nov 5, 2025

Further, note that with this change, the need for other functions is reduced, such as the functions that modify the priority of an element, or the function that looks for the largest priority, etc.

@dthain
Copy link
Member

dthain commented Nov 5, 2025

That sounds good to me.

@btovar
Copy link
Member Author

btovar commented Nov 5, 2025

@JinZhou5042 For example see the small change in vine_schedule.c

@btovar btovar requested a review from dthain November 6, 2025 13:36
This gives preference to older tasks to run first, and allows to
search in ready_tasks efficiently.
@btovar
Copy link
Member Author

btovar commented Nov 7, 2025

@JinZhou5042 I added the task id to the priority. This should allow looking for tasks in the ready task in O(logn) rather than O(n).

@JinZhou5042
Copy link
Member

@JinZhou5042 I added the task id to the priority. This should allow looking for tasks in the ready task in O(logn) rather than O(n).

Could you give an example of when this matters and why it is faster in this case?

@btovar
Copy link
Member Author

btovar commented Nov 7, 2025

Mainly when canceling tasks that are not running.

@JinZhou5042
Copy link
Member

But regardless we still need to scan the entire queue to find a task with a specific task_id?

@btovar
Copy link
Member Author

btovar commented Nov 7, 2025

No. With the taskid we know it's priorities and can look for it directly.

@btovar
Copy link
Member Author

btovar commented Nov 7, 2025

Well directly I mean logn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants