Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DB to prefer batches with preferred batch size for default max_queue_d… #3298

Merged
merged 2 commits into from
Sep 7, 2021

Conversation

tanmayv25
Copy link
Contributor

…elay

deadeyegoodwin
deadeyegoodwin previously approved these changes Aug 31, 2021
qa/L0_batcher/batcher_test.py Show resolved Hide resolved
@deadeyegoodwin
Copy link
Contributor

@tanmayv25 oops, didn't mean to approve yet. I had one requested change...

@@ -467,7 +464,7 @@ DynamicBatchScheduler::GetDynamicBatch()
return 0;
}

if (delay_is_exceeded) {
if (delay_is_exceeded || (pending_batch_delay_ns_ == 0)) {
Copy link
Contributor

@GuanLuo GuanLuo Aug 31, 2021

Choose a reason for hiding this comment

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

if pending_batch_delay_ns_ == 0, should the batcher NOT return 0 to wait for preferred batch or max batch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the question then becomes how long should it wait for forming the preferred batch size. These changes only let batcher prefer a preferred batch size if one can be formed. Otherwise it will issue the request with pending batches.

Copy link
Contributor

Choose a reason for hiding this comment

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

When delay is 0, the batcher should form the largest possible preferred batch, or if can't form any preferred batch then just the largest possible batch, and execute it immediately.

Tanmay, we should have a test with no delay where there are multiple preferred sizes and make sure that the largest preferred size is used, or when no preferred can be formed that the largest possible batch is formed. We already have these kind of tests but I guess they all use delay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am adding the tests...

@tanmayv25 tanmayv25 merged commit 51ae001 into main Sep 7, 2021
@tanmayv25 tanmayv25 deleted the tanmayv-fix branch September 7, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants