[SPARK-31069][CORE] high cpu caused by chunksBeingTransferred in external shuffle service by chrysan · Pull Request #27831 · apache/spark

chrysan · 2020-03-06T09:05:18Z

What changes were proposed in this pull request?

This change is targeted to speed up the calculation of chunksBeingTransferred to avoid high cpu when there are many stream requests.

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

… when iterating many streams

wangyum · 2020-03-06T09:11:31Z

ok to test.

SparkQA · 2020-03-06T12:17:54Z

Test build #119457 has finished for PR 27831 at commit d6af3f5.

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

Ngone51 · 2020-03-06T13:32:48Z

common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java

-    for (StreamState streamState: streams.values()) {
-      sum += streamState.chunksBeingTransferred.get();
-    }


Does this becomes a blocker? Or how much performance improvement we can gain in the new approach?

It makes no big different if the shuffle server handles not many chunks. While in our production environment, we found when the number of chunks reach 100,000 or more, sometimes most of the cpu resource are occupied by iterating and calculation the total number. Then no cpu resource to handle request and response data, which makes everything stuck.

Have you measured how long the thread is being stuck on calculation here? And how frequently this method is called?

It would be ideal to craft a simple benchmark code and experiment with some variations: num of chunks, contentions (no contention, one thread calling chunkSent, N threads doing the same).

Looks like the method is not called at only once, so the optimization seems to make sense. Still would be better to have some numbers to back up rationalization of the patch.

Ngone51 · 2020-03-06T13:33:46Z

common/network-common/src/main/java/org/apache/spark/network/server/OneForOneStreamManager.java

      StreamState state = entry.getValue();
      if (state.associatedChannel == channel) {
        streams.remove(entry.getKey());
+        totalChunksBeingTransferred.addAndGet((-state.chunksBeingTransferred.get()));


It is possible to get rid of state.chunksBeingTransferred totally?

No we can't because without it, we do not know how many to remove from total when channel terminated.

Maybe we can track transferring chunks by channel, e.g. Map[Channel, Count]? The number of channels should be less than streams.

And, if we can not get rid of state.chunksBeingTransferred, connectionTerminated could also be somewhat time consuming as it also traverse all streams.

And, if we can not get rid of state.chunksBeingTransferred, connectionTerminated could also be somewhat time consuming as it also traverse all streams.

IIUC, connectionTerminated won't be called frequently compared to chunksBeingTransferred which would less matter.

Oh, yeah. This a good point.

dongjoon-hyun · 2020-03-09T19:02:08Z

Retest this please.

SparkQA · 2020-03-09T21:31:46Z

Test build #119581 has finished for PR 27831 at commit d6af3f5.

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

HeartSaVioR · 2020-03-13T04:32:08Z

retest this, please

SparkQA · 2020-03-13T07:05:02Z

Test build #119744 has finished for PR 27831 at commit d6af3f5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-03-13T07:31:21Z

retest this, please

SparkQA · 2020-03-13T09:49:12Z

Test build #119748 has finished for PR 27831 at commit d6af3f5.

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

HeartSaVioR · 2020-03-13T09:53:10Z

@chrysan Could you please fill up the template form of PR? Providing number would be nice as I commented, but yes it seems obvious your patch helps for such case you've mentioned.

While in our production environment, we found when the number of chunks reach 100,000 or more, sometimes most of the cpu resource are occupied by iterating and calculation the total number.

jiangxb1987 · 2020-03-17T18:25:44Z

Please provide benchmark result of this change, thanks! Because this change introduces another stateful variable we need to maintain, that could potentially lead to logic conflicts. We should only accept this PR in case it does make significant performance difference.

HeartSaVioR · 2020-03-27T13:02:28Z

@chrysan Any update on this?

github-actions · 2020-07-06T00:30:08Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

…se hight cpu cost in external shuffle service when `maxChunksBeingTransferred` use default value ### What changes were proposed in this pull request? Followup from #27831 , origin author chrysan. Each request it will check `chunksBeingTransferred ` ``` public long chunksBeingTransferred() { long sum = 0L; for (StreamState streamState: streams.values()) { sum += streamState.chunksBeingTransferred.get(); } return sum; } ``` such as ``` long chunksBeingTransferred = streamManager.chunksBeingTransferred(); if (chunksBeingTransferred >= maxChunksBeingTransferred) { logger.warn("The number of chunks being transferred {} is above {}, close the connection.", chunksBeingTransferred, maxChunksBeingTransferred); channel.close(); return; } ``` It will traverse `streams` repeatedly and we know that fetch data chunk will access `stream` too, there cause two problem: 1. repeated traverse `streams`, the longer the length, the longer the time 2. lock race in ConcurrentHashMap `streams` In this PR, when `maxChunksBeingTransferred` use default value, we avoid compute `chunksBeingTransferred ` since we don't care about this. If user want to set this configuration and meet performance problem, you can also backport PR #27831 ### Why are the changes needed? Speed up getting `chunksBeingTransferred` and avoid lock race in object `streams` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existed UT Closes #30139 from AngersZhuuuu/SPARK-31069. Lead-authored-by: angerszhu <angers.zhu@gmail.com> Co-authored-by: chrysan <chrysanxia@gmail.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

speed up calculation of totalChunksBeingTransferred to avoid high cpu…

d6af3f5

… when iterating many streams

Ngone51 reviewed Mar 6, 2020

View reviewed changes

maropu changed the title ~~[SPARK-31069] high cpu caused by chunksBeingTransferred in external shuffle service~~ [SPARK-31069][CORE] high cpu caused by chunksBeingTransferred in external shuffle service Mar 6, 2020

dongjoon-hyun added the SPARK CORE label Mar 9, 2020

github-actions bot added the Stale label Jul 6, 2020

github-actions bot closed this Jul 7, 2020

HeartSaVioR mentioned this pull request Oct 23, 2020

[SPARK-31069][CORE] Avoid repeat compute chunksBeingTransferred cause hight cpu cost in external shuffle service when maxChunksBeingTransferred use default value. #30139

Closed

Conversation

chrysan commented Mar 6, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

wangyum commented Mar 6, 2020

Uh oh!

SparkQA commented Mar 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 9, 2020

Uh oh!

SparkQA commented Mar 9, 2020

Uh oh!

HeartSaVioR commented Mar 13, 2020

Uh oh!

SparkQA commented Mar 13, 2020

Uh oh!

HeartSaVioR commented Mar 13, 2020

Uh oh!

SparkQA commented Mar 13, 2020

Uh oh!

HeartSaVioR commented Mar 13, 2020

Uh oh!

jiangxb1987 commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeartSaVioR commented Mar 27, 2020

Uh oh!

github-actions bot commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HeartSaVioR Mar 13, 2020 •

edited

Loading

jiangxb1987 commented Mar 17, 2020 •

edited

Loading