You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "XinShuoWang (via GitHub)" <gi...@apache.org> on 2024/06/02 08:57:26 UTC
[PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
XinShuoWang opened a new pull request, #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951
## What changes were proposed in this pull request?
I used perf to observe the benchmark and found that the most time-consuming functions were `splitFixedWidthValueBuffer` and `splitBinaryType`. However, current column computing engines (such as starrocks) also use this idea of exchanging random read memory overhead for sequential write memory overhead to implement the `split` function, so I think there is not much room for optimization of the `split` function.
<img width="1213" alt="截屏2024-06-02 16 39 30" src="https://github.com/apache/incubator-gluten/assets/56379080/524dd7d2-1e91-4ccb-a928-23626a331736">
I found that when the ShuffleBatchSize is increased, the performance will be significantly improved. I think the performance benefits mainly come from the following aspects:
1. It can give full play to the advantages of sequential memory writing in the split stage. When PartitionNum is 10000 and ShuffleBatchSize is 4096 (the default value in the benchmark), each Partition is only allocated 1 row of data at most (the data obtained by logging statistics in the benchmark). At this time, it is obviously impossible to give full play to the advantages of sequential memory writing.
2. It can reduce the number of function calls and the number of memory allocations.
Therefore, I implemented this PR to cache the data to be Shuffled, which can optimize the performance of ShuffleWrite. For specific test data, please refer to the screenshot below.
I think this PR can also control whether to cache data in combination with memory usage, thereby avoiding the ShuffleWrite OOM problem.
## How was this patch tested?
### Command
```
./build/velox/benchmarks/shuffle_split_benchmark --file=/root/shuffleSplitBenchmark/cpp/velox/benchmarks/data/tpch_sf10m/lineitem/part-00000-6c374e0a-7d76-401b-8458-a8e31f8ab704-c000.snappy.parquet --partitions=10000 --iterations=100 --threads=1
```
### Before optimize
<img width="1775" alt="截屏2024-06-02 16 13 43" src="https://github.com/apache/incubator-gluten/assets/56379080/95a3d7a5-021a-4daa-abfe-aa9c96268077">
### After optimize
<img width="1778" alt="截屏2024-06-02 16 18 28" src="https://github.com/apache/incubator-gluten/assets/56379080/a989d3c3-3aa8-4b6c-9159-844f73044d33">
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2144099188
Thanks @XinShuoWang Please fix the code style so that we can run TPCH benchmark :)
> I think this PR can also control whether to cache data in combination with memory usage, thereby avoiding the ShuffleWrite OOM problem.
Is this part already included in the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2143761589
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICEGJ-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
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?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2373368545
Sounds great to me that someone could take on the remaining work of https://github.com/facebookincubator/velox/pull/7899. This batch resizing work doesn't have to be in individual operators. It's enough to see only some extra metrics showing the resizing time from relevant operators.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "HaoChen-ch (via GitHub)" <gi...@apache.org>.
HaoChen-ch commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2373201898
> The change makes sense to me. I think it's operational to merge this and use #6009 as follow-up which adds an individual Spark operator controlling this behavior for being reused for other operators in future (say, joins or aggs) by some kind of strategies. Let me know if any thoughts. @XinShuoWang @FelixYBW @marin-ma
velox has a feature to merge small vector for the output of aggs,filter and join，ref https://github.com/facebookincubator/velox/pull/7899/files but the pr is not merge to master. shall we go on the pr to merge it to master?
@zhztheplayer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2153866781
The change makes sense to me. I think it's operational to merge this and use #6009 as follow-up which adds an individual Spark operator controlling this behavior for being reused for other operators in future (say, joins or aggs) by some kind of strategies. Let me know if any thoughts @XinShuoWang @FelixYBW @marin-ma
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "XinShuoWang (via GitHub)" <gi...@apache.org>.
XinShuoWang commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2144509002
```
template <typename T>
arrow::Status splitFixedType(const uint8_t* srcAddr, const std::vector<uint8_t*>& dstAddrs) {
for (auto& pid : partitionUsed_) {
auto dstPidBase = (T*)(dstAddrs[pid] + partitionBufferBase_[pid] * sizeof(T));
// record dstPidBase
...
```
@FelixYBW I added a log at the above location and found that the interval between `dstPidBase` is about 16 KB. I think this can also be optimized to make the interval between `dstPidBase` very small, which will help reduce cache misses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "FelixYBW (via GitHub)" <gi...@apache.org>.
FelixYBW commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2144449307
Thank you for the improvement.
The ideal case of current split function is that: the input batch size should be as large as possible but all columns can fit into L2 cache. Once the column data can't fit into L2 cache, split performance will drop dramatically.
On the other side if the batch size is too small, the overhead of preparing of split actually can't be ignored. We should have room to improve with this case.
Also in current implementation, we must flatten the rowvector to split. The flatten itself also have overhead which is not analyzed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#discussion_r1623688580
##########
cpp/velox/shuffle/VeloxHashBasedShuffleWriter.h:
##########
@@ -249,6 +249,7 @@ class VeloxHashBasedShuffleWriter : public VeloxShuffleWriter {
template <typename T>
arrow::Status splitFixedType(const uint8_t* srcAddr, const std::vector<uint8_t*>& dstAddrs) {
+ ScopedTimer timer(&splitFixedTypeTimer_);
Review Comment:
Is this for debugging purposes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2144091204
/Benchmark Velox
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer merged PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "XinShuoWang (via GitHub)" <gi...@apache.org>.
XinShuoWang commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2156605562
@zhztheplayer @Yohahaha Can you help me to fix the failed CI? The error message is `java.lang.IllegalStateException: nativeShuffleWriter should not be -1L`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "Yohahaha (via GitHub)" <gi...@apache.org>.
Yohahaha commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2157567115
> @zhztheplayer @Yohahaha Can you help me to fix the failed CI? The error message is `java.lang.IllegalStateException: nativeShuffleWriter should not be -1L`
I think it's Uniffle's error and has been fixed on main branch, try rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Optimize the performance of hash based shuffle by accumulating batches [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #5951:
URL: https://github.com/apache/incubator-gluten/pull/5951#issuecomment-2153794304
Oops. I missed this PR before opening [this](https://github.com/apache/incubator-gluten/pull/6009) for similar purpose...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org