make maxCpuBatchSize in GpuPartitioning configurable #11929
Conversation
Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
binmahone
requested review from
abellina and
revans2
and removed request for
abellina
|
build |
| val SHUFFLE_PARTITIONING_MAX_CPU_BATCH_SIZE = | ||
| conf("spark.rapids.shuffle.partitioning.maxCpuBatchSize") | ||
| .doc("The maximum size of a sliced batch output to the CPU side " + | ||
| "when GPU partitioning shuffle data. This is used to limit the peak on-heap memory used " + |
There was a problem hiding this comment.
So one thing I would say was that this wasn't introduced to limit peak memory per se, it was introduced because we cannot go above this number (#45). If we are going to make it configurable, we should perhaps add a max check so we don't go above it. The comment could reflect this constraint.
+1 on the internal comment from @winningsix
There was a problem hiding this comment.
"it was introduced because we cannot go above this number " yes I'm also aware of this, but it happends that we can take advantage of it to limit peak memory. I changed the wording from "This is used to " to "This can be used to" to be more precise.
| @@ -1976,6 +1976,17 @@ val SHUFFLE_COMPRESSION_LZ4_CHUNK_SIZE = conf("spark.rapids.shuffle.compression. | |||
| .integerConf | |||
| .createWithDefault(20) | |||
|
|
|||
| val SHUFFLE_PARTITIONING_MAX_CPU_BATCH_SIZE = | |||
There was a problem hiding this comment.
copyright needs to be updated.
There was a problem hiding this comment.
Fixed. I have a stupid question: why don't we replace all the license header for all files in a batch? It's troublesome to take care of the license header every time.
Signed-off-by: Hongbin Ma (Mahone) <[email protected]>
This PR closes #11928