[VL] Use conf to control C2R occupied memory #5799
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
|
@jinchengchenghh @zhztheplayer Can you review this pr? |
XinShuoWang
changed the title
|
Run Gluten Clickhouse CI |
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala
Outdated
Show resolved
Hide resolved
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala
Outdated
Show resolved
Hide resolved
shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala
Outdated
Show resolved
Hide resolved
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxColumnarToRowExec.scala
Outdated
Show resolved
Hide resolved
cpp/core/jni/JniWrapper.cc
Outdated
| auto columnarToRowConverter = ctx->objectStore()->retrieve<ColumnarToRowConverter>(c2rHandle); | ||
| auto cb = ctx->objectStore()->retrieve<ColumnarBatch>(batchHandle); | ||
| columnarToRowConverter->convert(cb); | ||
|
|
||
| int64_t column2RowMemThreshold = 256 * 1024 * 1024; |
There was a problem hiding this comment.
Can you place this config computation to GlutenConfig.cc
|
Run Gluten Clickhouse CI |
|
I met similar issue recently, reports as ArrowContext OOM. Is the root cause that the RowVector too larger? or because the data in each row is too large? By default GLuten configure Velox as 4K batch, but looks many Velox operators may exceed 4K limit. Does the PR hold on the batch and output part of the batch each time? |
|
We may need couple of UTs to cover this |
|
cpp/core/jni/JniWrapper.cc
Outdated
| @@ -580,17 +583,28 @@ Java_org_apache_gluten_vectorized_NativeColumnarToRowJniWrapper_nativeColumnarTo | |||
| JNIEnv* env, | |||
| jobject wrapper, | |||
| jlong batchHandle, | |||
| jlong c2rHandle) { | |||
| jlong c2rHandle, | |||
| jlong rowId) { | |||
There was a problem hiding this comment.
Can we pass a range [startRowId, endRowId) to naitve ? I think it should be more clear to use the config at java side.
Can we use the batch size (4K row) by default as the config? We can still add the threshold but if customer doesn't config threshold, let's convert batch size (4k by default) instead of 256M memory. We can still reuse the memory allocated for row but I don't expect much perf gain here. It's too large for L1/L2 cache, very likely to be evicted from L3 as well when Spark process this row. |
|
Run Gluten Clickhouse CI |
|
why the PR is closed? |
Sorry, I accidentally forced push the main branch, which caused the PR to be closed. The new PR is here: #5952. |
What changes were proposed in this pull request?
In the current design, the Column2Row operation is completed in one go, which consumes a lot of memory and causes the program OOM. In this commit, I modified the C2R operation into multiple operations, which will greatly reduce the peak memory of the C2R operation. In addition, there should be some performance advantages from memory reuse.
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)