[SPARK-14242][CORE][Network] avoid copy in compositeBuffer for frame decoder #12038

liyezhang556520 · 2016-03-29T15:16:08Z

What changes were proposed in this pull request?

In this patch, we set the initial maxNumComponents to Integer.MAX_VALUE instead of the default size ( which is 16) when allocating compositeBuffer in TransportFrameDecoder because compositeBuffer will introduce too many memory copies underlying if compositeBuffer is with default maxNumComponents when the frame size is large (which result in many transport messages). For details, please refer to SPARK-14242.

How was this patch tested?

spark unit tests and manual tests.
For manual tests, we can reproduce the performance issue with following code:
sc.parallelize(Array(1,2,3),3).mapPartitions(a=>Array(new Array[Double](1024 * 1024 * 50)).iterator).reduce((a,b)=> a).length
It's easy to see the performance gain, both from the running time and CPU usage.

liyezhang556520 · 2016-03-29T15:18:47Z

common/network-common/src/main/java/org/apache/spark/network/util/TransportFrameDecoder.java

    }

-    // Otherwise, create a composite buffer.
-    CompositeByteBuf frame = buffers.getFirst().alloc().compositeBuffer();


actually we can set the maxNumComponents for compositeBuffer to avoid consolidate underlying, such as CompositeByteBuf frame = buffers.getFirst().alloc().compositeBuffer(Integer.MAX_VALUE);, but this might be not a good choice.

Why is it not a good choice? With your change, you're replacing "maybe copy multiple times" with "always copy once". If there's a way to avoid the copy altogether, why not do it?

@vanzin , I'm not sure why Netty underlying set a maximum number components (max size is Integer.MAX_VALUE), and the default value is only 16, this seems very small for consolidation. Will it occurs other problem when there are too many small buffers under compositeBuffer? Is that why it will consolidate when the small buffer number reaches the maxNumCompnent?

SparkQA · 2016-03-29T17:24:34Z

Test build #54441 has finished for PR 12038 at commit 8908585.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-03-29T20:01:50Z

Maybe we can try to use CompositeByteBuf.addComponents? So that we can avoid to do the copy when the size of buffers are not more than 16.

zsxwing · 2016-03-29T20:01:54Z

cc @vanzin

liyezhang556520 · 2016-03-30T02:49:33Z

@vanzin , I think @zsxwing 's idea of using CompositeByteBuf.addComponents is a better choice, which will only introduce exactly one copy if the small buffer number is lager than 16 and will not introduce any copy if that less than 16. Let me update this PR first.

SparkQA · 2016-03-30T06:41:15Z

Test build #54492 has finished for PR 12038 at commit 1eacf55.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-03-30T16:47:14Z

will only introduce exactly one copy if the small buffer number is lager than 16

That's better, but is it needed at all? I don't see any comments about why consolidating the buffers is a win in the source for CompositeByteBuf. Traversing the single buffer should be slightly faster because there's less bookkeeping, but there's the cost of copying that data in the first place.

When testing this code, I remember that during large transfers packets would arrive in 64k chunks at the most, so that means that once you're transferring more than 1MB, you'd have to copy things.

Have you tried not consolidating to see whether there's any negative side-effect?

liyezhang556520 · 2016-03-31T02:02:14Z

@vanzin

That's better, but is it needed at all? I don't see any comments about why consolidating the buffers is a win in the source for CompositeByteBuf. Traversing the single buffer should be slightly faster because there's less bookkeeping, but there's the cost of copying that data in the first place.

If so we can just set the maxNumComponents with Integer.Max_VALUE for compositeByteBuffer.

When testing this code, I remember that during large transfers packets would arrive in 64k chunks at the most, so that means that once you're transferring more than 1MB, you'd have to copy things.

In my test, the chunk sizes are mainly around 20~30 KB.

Have you tried not consolidating to see whether there's any negative side-effect?

I tested previously with buffers.getFirst().alloc().compositeBuffer(Integer.MAX_VALUE);, and with frame size over 1GB, which consist of about 40000 chunks, I didn't see negative side-effect.

Ok, let's solve this issue without copy for any case.

SparkQA · 2016-03-31T02:34:53Z

Test build #54578 has finished for PR 12038 at commit 80f7573.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

liyezhang556520 · 2016-03-31T02:39:10Z

retest this please.

SparkQA · 2016-03-31T04:39:40Z

Test build #54580 has finished for PR 12038 at commit 80f7573.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-03-31T16:58:25Z

@liyezhang556520 looks great but could you update the commit message to reflect the actual change? Thanks

zsxwing · 2016-03-31T19:55:44Z

LGTM @liyezhang556520 please ping me when you update the PR description.

liyezhang556520 · 2016-04-01T01:31:53Z

@zsxwing , I updated the commit description. Thank you @zsxwing and @vanzin for reviewing.

zsxwing · 2016-04-01T03:17:21Z

Merging to master. Thanks, @liyezhang556520 !

…rame decoder apache#12038 [EXT][SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle rule apache#11438

davies · 2016-04-11T06:38:00Z

cherry-picked into 1.6 branch

zzcclp · 2016-04-11T23:06:14Z

@davies has this pr cherry-picked into branch-1.6?

davies · 2016-04-11T23:17:43Z

Yes

zzcclp · 2016-04-12T01:24:34Z

@davies , but I didn't find this commit in branch-1.6.

liyezhang556520 · 2016-04-12T01:40:59Z

@davies , I didn't see the commit in branch-1.6 either, seems this commit can not be simply git cherry-pick because the file path is not the same in master and branch-1.6. Do I need to submit another PR for back-port?

…decoder ## What changes were proposed in this pull request? In this patch, we set the initial `maxNumComponents` to `Integer.MAX_VALUE` instead of the default size ( which is 16) when allocating `compositeBuffer` in `TransportFrameDecoder` because `compositeBuffer` will introduce too many memory copies underlying if `compositeBuffer` is with default `maxNumComponents` when the frame size is large (which result in many transport messages). For details, please refer to [SPARK-14242](https://issues.apache.org/jira/browse/SPARK-14242). ## How was this patch tested? spark unit tests and manual tests. For manual tests, we can reproduce the performance issue with following code: `sc.parallelize(Array(1,2,3),3).mapPartitions(a=>Array(new Array[Double](1024 * 1024 * 50)).iterator).reduce((a,b)=> a).length` It's easy to see the performance gain, both from the running time and CPU usage. Author: Zhang, Liye <[email protected]> Closes #12038 from liyezhang556520/spark-14242.

davies · 2016-04-12T02:25:49Z

sorry, I forgot to push. it's in branch-1.6 now.

…decoder In this patch, we set the initial `maxNumComponents` to `Integer.MAX_VALUE` instead of the default size ( which is 16) when allocating `compositeBuffer` in `TransportFrameDecoder` because `compositeBuffer` will introduce too many memory copies underlying if `compositeBuffer` is with default `maxNumComponents` when the frame size is large (which result in many transport messages). For details, please refer to [SPARK-14242](https://issues.apache.org/jira/browse/SPARK-14242). spark unit tests and manual tests. For manual tests, we can reproduce the performance issue with following code: `sc.parallelize(Array(1,2,3),3).mapPartitions(a=>Array(new Array[Double](1024 * 1024 * 50)).iterator).reduce((a,b)=> a).length` It's easy to see the performance gain, both from the running time and CPU usage. Author: Zhang, Liye <[email protected]> Closes apache#12038 from liyezhang556520/spark-14242. (cherry picked from commit 663a492)

spark-14242 avoid using compositeBuffer for frame decoder

8908585

liyezhang556520 reviewed Mar 29, 2016
View reviewed changes

use compositeByteBuf.addComponents to for netty frame buffer

1eacf55

liyezhang556520 changed the title ~~[SPARK-14242][CORE][Network] avoid using compositeBuffer for frame decoder~~ [SPARK-14242][CORE][Network] avoid too many copies in compositeBuffer for frame decoder Mar 30, 2016

only set maxNumComponents for compositeByteBuffer to solve the issue

80f7573

liyezhang556520 changed the title ~~[SPARK-14242][CORE][Network] avoid too many copies in compositeBuffer for frame decoder~~ [SPARK-14242][CORE][Network] avoid copy in compositeBuffer for frame decoder Mar 31, 2016

asfgit closed this in 96941b1 Apr 1, 2016

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Apr 1, 2016

[EXT][SPARK-14242][CORE][Network] avoid copy in compositeBuffer for f…

455edc5

…rame decoder apache#12038 [EXT][SPARK-13583][CORE][STREAMING] Remove unused imports and add checkstyle rule apache#11438

liyezhang556520 deleted the spark-14242 branch April 11, 2016 08:16

srowen mentioned this pull request Feb 2, 2019

[SPARK-26674][CORE]Consolidate CompositeByteBuf when reading large frame #23602

Closed

[SPARK-14242][CORE][Network] avoid copy in compositeBuffer for frame decoder #12038

[SPARK-14242][CORE][Network] avoid copy in compositeBuffer for frame decoder #12038

Uh oh!

Conversation

liyezhang556520 commented Mar 29, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

liyezhang556520 Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

liyezhang556520 Mar 30, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 29, 2016

Uh oh!

zsxwing commented Mar 29, 2016

Uh oh!

zsxwing commented Mar 29, 2016

Uh oh!

liyezhang556520 commented Mar 30, 2016

Uh oh!

SparkQA commented Mar 30, 2016

Uh oh!

vanzin commented Mar 30, 2016

Uh oh!

liyezhang556520 commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

liyezhang556520 commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

vanzin commented Mar 31, 2016

Uh oh!

zsxwing commented Mar 31, 2016

Uh oh!

liyezhang556520 commented Apr 1, 2016

Uh oh!

zsxwing commented Apr 1, 2016

Uh oh!

davies commented Apr 11, 2016

Uh oh!

zzcclp commented Apr 11, 2016

Uh oh!

davies commented Apr 11, 2016

Uh oh!

zzcclp commented Apr 12, 2016

Uh oh!

liyezhang556520 commented Apr 12, 2016

Uh oh!

davies commented Apr 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants