Skip to content

Conversation

@colinthomas-z80
Copy link
Contributor

@colinthomas-z80 colinthomas-z80 commented Nov 11, 2025

Proposed Changes

Addresses #4276

We cannot partially iterate the ready list and leave it out of order if we wish to maintain priority scheduling. If a task priority is set by the user, revert to iterating the entire list each scheduling attempt.

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.

Copy link
Member

@btovar btovar left a comment

Choose a reason for hiding this comment

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

ready to merge?

@dthain
Copy link
Member

dthain commented Nov 11, 2025

Hold on a minute...

@dthain
Copy link
Member

dthain commented Nov 11, 2025

I don't think send_one_task has the desired effect. If no tasks are schedulable, it will rotate through the entire list, yes. But if it starts working through the list and finds one halfway, then it will return after rotating half the list, and the priority order is lost. Or am I misunderstanding something?

@colinthomas-z80
Copy link
Contributor Author

You're right I think. Maybe I can just switch back to list_iterate

@btovar
Copy link
Member

btovar commented Nov 11, 2025

list_rotate was added for performance advantages, right?
Would it be better to keep it and count how many rotations where done and then rotate back to the start if needed?

@colinthomas-z80
Copy link
Contributor Author

That could get complicated quickly if we are pushing new tasks with priorities while the list is out of order

@btovar
Copy link
Member

btovar commented Nov 11, 2025

We can avoid the code duplication if you use a function pointer for whether to use list_nextitem or list_rotate, and update q->attempt_schedule_depth to the length of the ready_queue every time when using priorities.

@colinthomas-z80
Copy link
Contributor Author

Is it necessary to call list_first_item before iterating the list normally?

@dthain
Copy link
Member

dthain commented Nov 11, 2025

Careful! You are iterating over the list but using list_pop_tail to remove a task. Those don't match up.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants