Skip to content

[rush] cobuilds: yield priority to other tasks in the queue#5741

Open
aramissennyeydd wants to merge 3 commits intomicrosoft:mainfrom
aramissennyeydd:sennyeya/fix-cobuild-ordering
Open

[rush] cobuilds: yield priority to other tasks in the queue#5741
aramissennyeydd wants to merge 3 commits intomicrosoft:mainfrom
aramissennyeydd:sennyeya/fix-cobuild-ordering

Conversation

@aramissennyeydd
Copy link
Copy Markdown
Contributor

Summary

#4821 updated the behavior of cobuilds such that large weight operations (such as those that lock and entire agent) do not get skipped while they're remote executing. Instead, they force the agent that's waiting on them to continue reading their state (with only a 500ms window to pick up another task). This PR attempts to improve operation scheduling by moving tasks that are currently cobuilding to the back of the queue and let other tasks go in front.

Details

I don't love the API addition here, happy to update as needed. Just wanted to plumb through the idea of moving an operation to the back of the queue. This could also be done on Operation.status change automatically.

How it was tested

Added unit tests. Would love to get a preview version to test on our internal repo.

Impacted documentation

Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
@aramissennyeydd
Copy link
Copy Markdown
Contributor Author

@dmichon-msft Curious your thoughts here - we're seeing much longer cobuilds than we expect and I think it's related to this.

Signed-off-by: Aramis Sennyey <aramissennyeydd@users.noreply.github.com>
// Tracks how many times each operation has been assigned to an execution slot.
// Operations that have been assigned more times (e.g. cobuild retries) are sorted
// after operations with fewer attempts, so untried work is preferred.
private readonly _timesQueued: Map<OperationExecutionRecord, number>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
private readonly _timesQueued: Map<OperationExecutionRecord, number>;
private readonly _numberOfTimesQueuedByOperation: Map<OperationExecutionRecord, number>;

Comment on lines +25 to +27
// Tracks how many times each operation has been assigned to an execution slot.
// Operations that have been assigned more times (e.g. cobuild retries) are sorted
// after operations with fewer attempts, so untried work is preferred.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Tracks how many times each operation has been assigned to an execution slot.
// Operations that have been assigned more times (e.g. cobuild retries) are sorted
// after operations with fewer attempts, so untried work is preferred.
/**
* Tracks how many times each operation has been assigned to an execution slot.
* Operations that have been assigned more times (e.g. cobuild retries) are sorted
* after operations with fewer attempts, so untried work is preferred.
*/

Comment on lines +192 to +197
const r2: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r2.value);
const r3: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r3.value);
const r4: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r4.value);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const r2: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r2.value);
const r3: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r3.value);
const r4: IteratorResult<OperationExecutionRecord> = await queue.next();
results.push(r4.value);
for await (const item of queue) {
result.push(item);
}

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

Labels

None yet

Projects

Status: Needs triage

Development

Successfully merging this pull request may close these issues.

2 participants