[SYCL][PI][L0] Add dynamic batch size adjustment #2792
Conversation
Pull to update my local fork
updating fork from main
update fork to sycl head
Update my fork
Updating fork to head of sycl.
Update to sycl head.
update my fork to sycl head
These changes add code to implement dynamic command list batch size adjustment. This adds code that adjusts the batch size based on how often the command list batch is closed and executed when full versus how often it is closed and executed only partially full. This also adds a new environment variable SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH, and its accompanying documentation.
kbsmith-intel
requested review from
kbobrovs,
pvchupin and
smaslov-intel
as code owners
sycl/doc/EnvironmentVariables.md
Outdated
| | SYCL_PI_LEVEL_ZERO_DISABLE_USM_ALLOCATOR | Any(\*) | Disable USM allocator in Level Zero plugin (each memory request will go directly to Level Zero runtime) | | ||
| | SYCL_PI_LEVEL_ZERO_BATCH_SIZE | Positive integer | Sets a preferred number of commands to batch into a command list before executing the command list. Values 0 and 1 turn off batching. Default is 4. | | ||
| | SYCL_PI_LEVEL_ZERO_BATCH_SIZE | Positive integer | Sets a preferred number of commands to batch into a command list before executing the command list. Value of 0 turns off batching. Default is 4. | | ||
| | SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH | Any(\*) | Disable dyanmic command list batch size adjustment. | |
There was a problem hiding this comment.
| | SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH | Any(\*) | Disable dyanmic command list batch size adjustment. | | |
| | SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH | Any(\*) | Disable dynamic command list batch size adjustment. | |
|
Please only review this pull request. Please do not merge it quite yet. |
|
/summary:run |
This changes the level zero plug-in command list batching unit test to turn off new dyanmic batching feature. The test was intended to test basic batching functionality with a fixed batch size, and these changes make that continue to be true.
This change adds a new unit test which tests the dynamic batch size adjustment feature. This test is likely to need to be updated if the batch size adjustment algorithm changes.
This removes some extra en-of-line whitespace that was found during clang-format run on new level_zero_dynamic_batch_test.cpp test.
pvchupin
changed the title
Updated PR title with tag so it's not merged accidentally. |
This commit updates the dynamic batching code, documentation, and test cases to address review comments. This deletes the newly added environment variable SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH. Instead, this uses the SYCL_PI_LEVEL_ZERO_BATCH_SIZE=0 to mean use dyanmic batching and retains all the positive values above zero for menaing use fixed size batching of that exact value. the default is changed to use dyanmic batching. This also slightly updates the interface to update the dynamic batch size in a way I thought was a little cleaner.
This change lowers the maximum dynamic batch size to 16 from 32, and starts the batch size for dynamic batching at 2. It updates the dynamic batch test output as needed because of the start batch size change.
| } | ||
|
|
||
| void _pi_queue::adjustBatchSizeForFullBatch() { | ||
| // QueueBatchSize of 0 means never allow batching. |
There was a problem hiding this comment.
NIT: I think it can't happen in today's code the QueueBatchSize is equal to 0
There was a problem hiding this comment.
That is incorrect. In the queue itself, QueueBatchSize of 0 means - No Batching. Because I need to allow QueueBatchSize to be 1 when dynamic batching is being used, because the possibility is that dynamic batching can adjust QueueBatchSize down to as low as 1 if we are still doing too many partial batches. So, in the queue itself, QueueBatchSize == 0 means no batching, QueueBatchSize > 0 is just the current bacthing size, and the queue bool UseDynamicBatching controls whether dynamic batch adjustment ever changes QueueBatchSize up or down.
There was a problem hiding this comment.
So, in the queue itself, QueueBatchSize == 0 means no batching
I realized this, yeah. But the code (in _pi_queue constructor) seems to never set it to 0.
There was a problem hiding this comment.
On looking at the code in pi_level_zero.hpp, I see that you are correct. Batching turned off is now really
represented as QueueBatchSize == 1 and UseDynamicBatching== false, or to say it another way, batching is turned off by using a fixed batch size of 1. Thank you for pointing that out.
kbsmith-intel
changed the title
|
I changed the title as I think this is now ready to be merged after all reviews have completed. |
|
@kbsmith-intel, I'd like to squash and merge, could you please post a new commit message you'd like me to capture for a squashed commit? There've been changes done during code review and I'd like to capture summary of this change as accurate as possible. |
|
OK, here's the commit log message I would like: These changes add code to implement dynamic command list batch |
These changes add code to implement dynamic command list batch
size adjustment. This adds code that adjusts the batch size based
on how often the command list batch is closed and executed when
full versus how often it is closed and executed only partially full.
This also adds a new environment variable
SYCL_PI_LEVEL_ZERO_DISABLE_DYNAMIC_BATCH, and its accompanying
documentation.