[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. #12060

ueshin · 2016-03-30T09:42:01Z

What changes were proposed in this pull request?

Currently a method submitStage() for waiting stages is called on every iteration of the event loop in DAGScheduler to submit all waiting stages, but most of them are not necessary because they are not related to Stage status.
The case we should try to submit waiting stages is only when their parent stages are successfully completed.

This elimination can improve DAGScheduler performance.

How was this patch tested?

Added some checks and other existing tests, and our projects.

We have a project bottle-necked by DAGScheduler, having about 2000 stages.

Before this patch the almost all execution time in Driver process was spent to process submitStage() of dag-scheduler-event-loop thread but after this patch the performance was improved as follows:

	total execution time	`dag-scheduler-event-loop` thread time	`submitStage()`
Before	760 sec	710 sec	667 sec
After	440 sec	14 sec	10 sec

…cal order to ensure building ancestor stages first.

This reverts commit 1636531.

ueshin · 2016-03-30T09:42:21Z

This PR is based on #11720, so please check it first.

SparkQA · 2016-03-30T11:55:02Z

Test build #54509 has finished for PR 12060 at commit a304235.

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

maropu · 2016-03-30T14:57:33Z

Seems that this optimization must need correct stage graphs fixed in #11720, is this correct?

ueshin · 2016-03-30T16:11:36Z

Yes, that's right.
#11720 is needed to find child stages correctly.

maropu · 2016-03-30T16:18:51Z

If so, it'd be better to close #11720; the benefit of correct stage graphs fixed in #11720 seems to be clear in this pr.

markhamstra · 2016-03-30T19:27:40Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

@@ -1247,7 +1252,7 @@ class DAGScheduler(
                }
              }

-              // Note: newly runnable stages will be submitted below when we submit waiting stages
+              submitWaitingChildStages(shuffleStage)


Should this be done when !shuffleStage.isAvailable and we have resubmitted the shuffleStage, or only within the else branch?

@markhamstra Thank you for your review.
Definitely we can move this into else branch.
I'll modify it.

…ge becomes available.

ueshin · 2016-03-31T03:20:14Z

@maropu I see. I'll close #11720.

SparkQA · 2016-03-31T04:44:01Z

Test build #54582 has finished for PR 12060 at commit f1407c0.

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

ueshin · 2016-03-31T05:00:31Z

Jenkins, retest this please.

SparkQA · 2016-03-31T06:56:14Z

Test build #54595 has finished for PR 12060 at commit f1407c0.

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

maropu · 2016-04-01T07:13:00Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

-    for (stage <- waitingStagesCopy.sortBy(_.firstJobId)) {
+    val childStages = waitingStages.filter(_.parents.contains(parent)).toArray
+    waitingStages --= childStages
+    for (stage <- childStages.sortBy(_.firstJobId)) {
      submitStage(stage)


Seems submitWaitingChildStages is called to submit child stages when the given parent stage is available. From this observation, do we have to re-check missing parents inside submitStage?

Yes, and the re-check is done in the submitStage().
If there are some missing parent stages, the child will go to waitingStages again.

Ahah, I see.

maropu · 2016-04-01T08:50:45Z

This pr seems great in terms of spark-core performance, so could you assign qualified guys to review this? cc: @rxin

SparkQA · 2016-05-13T02:09:51Z

Test build #58529 has finished for PR 12060 at commit 0c0d9ed.

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

zzcclp · 2016-05-13T03:16:03Z

Good PR, will it plan to be merged into branch-1.6?

markhamstra · 2016-05-13T03:48:15Z

@zzcclp Not likely. This PR shouldn't produce any different results, but rather produces the same results faster. We're typically very conservative with patch-level releases, so the optimization work for this PR will almost certainly only appear in the Spark 2.x series. That's not too far off.

zzcclp · 2016-05-13T04:09:08Z

@markhamstra , thanks for your explaintion.

kayousterhout · 2016-05-16T23:22:04Z

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala

@@ -1357,7 +1345,6 @@ class DAGScheduler(
      logDebug("Additional executor lost message for " + execId +
               "(epoch " + currentEpoch + ")")
    }


Is it necessary to submit some newly-waiting stages here (e.g., if shuffle output was lost for a map stage, so now that map stage needs to be re-run)?

This appears to be a non-issue, because we handle lost shuffle output separately, when we get a FetchFailure from a task that tries to fetch the output.

kayousterhout · 2016-05-16T23:54:08Z

This LGTM with the small comment changes I suggested. @markhamstra any objections to this? Mark / @rxin thoughts on merging it into the 2.0 branch?

ueshin · 2016-05-17T01:17:37Z

@kayousterhout Thank you for your review.
I updated the comments and pushed.

SparkQA · 2016-05-17T03:07:28Z

Test build #58659 has finished for PR 12060 at commit 4eb8c05.

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

SparkQA · 2016-05-23T20:27:33Z

Test build #59133 has finished for PR 12060 at commit 4eb8c05.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2016-05-23T20:53:26Z

Jenkins, retest this please.

SparkQA · 2016-05-23T22:51:35Z

Test build #59153 has finished for PR 12060 at commit 4eb8c05.

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

kayousterhout · 2016-05-25T20:58:58Z

I merged this into master (not 2.0, since it's a performance problem rather than a correctness problem, and this isn't a regression). Thanks @ueshin!

ueshin · 2016-05-25T22:28:56Z

Thanks a lot for merging this!

apache#12060

Currently a method `submitStage()` for waiting stages is called on every iteration of the event loop in `DAGScheduler` to submit all waiting stages, but most of them are not necessary because they are not related to Stage status. The case we should try to submit waiting stages is only when their parent stages are successfully completed. This elimination can improve `DAGScheduler` performance. Added some checks and other existing tests, and our projects. We have a project bottle-necked by `DAGScheduler`, having about 2000 stages. Before this patch the almost all execution time in `Driver` process was spent to process `submitStage()` of `dag-scheduler-event-loop` thread but after this patch the performance was improved as follows: | | total execution time | `dag-scheduler-event-loop` thread time | `submitStage()` | |--------|---------------------:|---------------------------------------:|----------------:| | Before | 760 sec | 710 sec | 667 sec | | After | 440 sec | 14 sec | 10 sec | Author: Takuya UESHIN <ueshin@happy-camper.st> Closes apache#12060 from ueshin/issues/SPARK-14269.

## What changes were proposed in this pull request? Currently a method `submitStage()` for waiting stages is called on every iteration of the event loop in `DAGScheduler` to submit all waiting stages, but most of them are not necessary because they are not related to Stage status. The case we should try to submit waiting stages is only when their parent stages are successfully completed. This elimination can improve `DAGScheduler` performance. ## How was this patch tested? Added some checks and other existing tests, and our projects. We have a project bottle-necked by `DAGScheduler`, having about 2000 stages. Before this patch the almost all execution time in `Driver` process was spent to process `submitStage()` of `dag-scheduler-event-loop` thread but after this patch the performance was improved as follows: | | total execution time | `dag-scheduler-event-loop` thread time | `submitStage()` | |--------|---------------------:|---------------------------------------:|----------------:| | Before | 760 sec | 710 sec | 667 sec | | After | 440 sec | 14 sec | 10 sec | Author: Takuya UESHIN <ueshin@happy-camper.st> Closes apache#12060 from ueshin/issues/SPARK-14269.

ueshin added 14 commits March 15, 2016 17:46

Add a test to check if the stage graph is properly built.

9a1724d

Make DAGScheduler.getAncestorShuffleDependencies() return in topologi…

f8b7910

…cal order to ensure building ancestor stages first.

Refactor getAncestorShuffleDependencies.

0ea3fc8

Fix topological sort.

697b322

Merge branch 'master' into issues/SPARK-13902

d6d3c34

Add assertion to check not to overwrite illegally.

1636531

Modify to mitigate adds extra push&pop.

92e9f44

Modify comment.

4b412f5

Add a comment to explain what the test is doing.

8fb9a14

Revert "Add assertion to check not to overwrite illegally."

e2cfeaf

This reverts commit 1636531.

Eliminate unnecessary submitWaitingStages() call.

e3c0de3

Merge branch 'issues/SPARK-13902' into issues/SPARK-14269

b73eaac

Add some checks.

88c4bc1

Try to submit only child stages of the completed stage.

a304235

ueshin mentioned this pull request Mar 30, 2016

[SPARK-13902][SCHEDULER] Make DAGScheduler.getAncestorShuffleDependencies() return in topological order to ensure building ancestor stages first. #11720

Closed

markhamstra reviewed Mar 30, 2016
View reviewed changes

Modify to call submitWaitingChildStages only when the completed sta…

f1407c0

…ge becomes available.

ueshin changed the title ~~[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call.~~ [SPARK-13902][SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. Mar 31, 2016

maropu reviewed Apr 1, 2016
View reviewed changes

ueshin mentioned this pull request Apr 25, 2016

[SPARK-13902][SCHEDULER] Make DAGScheduler not to create duplicate stage. #12655

Closed

ueshin added 2 commits May 13, 2016 09:06

Revert DAGScheduler.getAncestorShuffleDependencies.

eb230bb

Merge branch 'master' into issues/SPARK-14269

0c0d9ed

ueshin changed the title ~~[SPARK-13902][SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call.~~ [SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. May 13, 2016

kayousterhout reviewed May 16, 2016
View reviewed changes

Update comments.

4eb8c05

asfgit closed this in 698ef76 May 25, 2016

zzcclp added a commit to zzcclp/spark that referenced this pull request May 26, 2016

[EXT][SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call.

d664a06

apache#12060

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. #12060

[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. #12060

ueshin commented Mar 30, 2016

ueshin commented Mar 30, 2016

SparkQA commented Mar 30, 2016

maropu commented Mar 30, 2016

ueshin commented Mar 30, 2016

maropu commented Mar 30, 2016

markhamstra Mar 30, 2016

ueshin Mar 31, 2016

ueshin commented Mar 31, 2016

SparkQA commented Mar 31, 2016

ueshin commented Mar 31, 2016

SparkQA commented Mar 31, 2016

maropu Apr 1, 2016

ueshin Apr 1, 2016

maropu Apr 1, 2016

maropu commented Apr 1, 2016

SparkQA commented May 13, 2016

zzcclp commented May 13, 2016

markhamstra commented May 13, 2016

zzcclp commented May 13, 2016

kayousterhout May 16, 2016

kayousterhout May 16, 2016

kayousterhout commented May 16, 2016

ueshin commented May 17, 2016

SparkQA commented May 17, 2016

SparkQA commented May 23, 2016

ueshin commented May 23, 2016

SparkQA commented May 23, 2016

kayousterhout commented May 25, 2016

ueshin commented May 25, 2016

[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. #12060

[SPARK-14269][SCHEDULER] Eliminate unnecessary submitStage() call. #12060

Conversation

ueshin commented Mar 30, 2016

What changes were proposed in this pull request?

How was this patch tested?

ueshin commented Mar 30, 2016

SparkQA commented Mar 30, 2016

maropu commented Mar 30, 2016

ueshin commented Mar 30, 2016

maropu commented Mar 30, 2016

markhamstra Mar 30, 2016

Choose a reason for hiding this comment

ueshin Mar 31, 2016

Choose a reason for hiding this comment

ueshin commented Mar 31, 2016

SparkQA commented Mar 31, 2016

ueshin commented Mar 31, 2016

SparkQA commented Mar 31, 2016

maropu Apr 1, 2016

Choose a reason for hiding this comment

ueshin Apr 1, 2016

Choose a reason for hiding this comment

maropu Apr 1, 2016

Choose a reason for hiding this comment

maropu commented Apr 1, 2016

SparkQA commented May 13, 2016

zzcclp commented May 13, 2016

markhamstra commented May 13, 2016

zzcclp commented May 13, 2016

kayousterhout May 16, 2016

Choose a reason for hiding this comment

kayousterhout May 16, 2016

Choose a reason for hiding this comment

kayousterhout commented May 16, 2016

ueshin commented May 17, 2016

SparkQA commented May 17, 2016

SparkQA commented May 23, 2016

ueshin commented May 23, 2016

SparkQA commented May 23, 2016

kayousterhout commented May 25, 2016

ueshin commented May 25, 2016