vine: check worker has committable resources #4039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
btovar
approved these changes
Jan 27, 2025
|
Looks good. Are you still testing it? |
|
Yes, I am going to do some large tests to make sure they work well. |
7 tasks
dthain
requested changes
Jan 28, 2025
JinZhou5042
changed the title
JinZhou5042
changed the title
JinZhou5042
changed the title
btovar
requested changes
Feb 6, 2025
taskvine/src/manager/vine_schedule.c
Outdated
| * @param resource @vine_resources.h:struct vine_resources | ||
| * @return 1 if the resource type is defined and can be allocated to a task, 0 otherwise. | ||
| */ | ||
| static int is_resource_committable(struct vine_manager *q, struct vine_resource resource) |
There was a problem hiding this comment.
You are unnecessarily copying the struct vine_resource data. In this case, I think it will be clearer if you do the resource.total > 0 && resource.inuse < overcommitted_resource_total(q, resource.total); per resource inside check_worker_have_committable_resources.
taskvine/src/manager/vine_schedule.c
Outdated
| /* If any slots are committable */ | ||
| uint64_t task_id; | ||
| struct vine_task *t; | ||
| ITABLE_ITERATE(w->current_libraries, task_id, t) |
There was a problem hiding this comment.
Maybe first check that itable_size > 0 before setting the iteration?
btovar
approved these changes
Feb 6, 2025
|
ready to merge? |
|
RTM |
btovar
pushed a commit
that referenced
this pull request
Feb 6, 2025
* init * move empty libraries substract * rename to is_resource_fully_allocated * handle has_cores && has_gpus * check libraries * simplify the code * remove from the data structure * revert * remove is_resource_committable
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
An improved version suggested by @btovar in #4037
The issue is that even though most allocations work fine in #4035 and tasks run pretty densely, the manager still wastes a lot of time checking tasks when there aren’t any usable cores.
Task scheduling seems to be an expensive part of the main loop, we could save a lot of time by making this more efficient, and then the manager has more time to do other jobs like output retrieving and temp file replication.
The most common issue in
check_worker_against_taskis that the worker doesn’t have a free core. But it still goes through the whole resource allocation process, and this keeps happening for a bunch of tasks each time, based on the depth set byq->attempt_schedule_depth.This PR adds a tag for workers to note whether they have any available resources. This allows the checking process to terminate earlier.
Merge Checklist
The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.