[SPARK-7795] [Core] Speed up task scheduling in standalone mode by reusing serializer

[coolfrood]

My experiments with scheduling very short tasks in standalone cluster mode indicated that a significant amount of time was being spent in scheduling the tasks (>500ms for 256 tasks). I found that most of the time was being spent in creating a new instance of serializer for each task. Changing this to just one serializer brought down the scheduling time to 8ms.

[srowen]

@coolfrood have a look at https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark -- you missed a step or two. It's not 100% clear you can reuse the serializer but it is a good idea to float.

[coolfrood]

Argh. Sorry. I'll create a JIRA for this and update the PR.

[JoshRosen]

Serializer instances aren't thread-safe, but it looks like this single-threaded use should be fine. I implemented some similar optimizations in #5606 and am in the process of fixing a rare corner-case related to serializer re-use with uncommon Kryo configurations (#6293). Based on discussion in those patches, this change looks good to me.

We might actually be able to go a bit further and lift the serializer instance into a field of CoarseGrainedSchedulerBackend itself. That would be a bit riskier in terms of code review burden, since we'd have to double-check that CoarseGrainedSchedulerBackend is only ever used from a single thread (I think that this is actually the case, but would be good to double-check).

[JoshRosen]

Jenkins, this is ok to test.

[coolfrood]

@JoshRosen: it looks like launchTasks() is only called from makeOffers(), which in turn is only called from the Actor's receive, so it should be safe to move the serializer into a member field. Also, both launchTasks() and makeOffers() can be made private. What do you think?

[JoshRosen]

Yep, looks like both launchTasks() and makeOffers() can be private. In general, I prefer to minimize the visibility of fields / methods as much as possible, so let's make these private.

My PR made a similar change to CoarseGrainedExecutorBackend. Over there, I added a comment to explain that the shared serializer code would need to be revisited if we ever switched to a parallel / non-thread-safe RPC endpoint for the actor: https://github.com/apache/spark/pull/5606/files#diff-79391110e9f26657e415aa169a004998R51. It would be good to make a similar comment here so that we don't forget to update this if DriverEndpoint is ever parallelized.

[SparkQA]

Test build #33263 has finished for PR 6323 at commit 0b8ca93.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #33259 has finished for PR 6323 at commit fe530cd.

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

[SparkQA]

Test build #33266 has finished for PR 6323 at commit bd4a5dd.

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

[JoshRosen]

This looks good to me. I'm going to loop in @zsxwing or @rxin for another pair of eyes / final sanity-check.

[zsxwing]

LGTM

[JoshRosen]

Actually, one minor comment: ser currently has wider visibility than is strictly necessary: it's only used inside of the DriverEndpoint inner class, so it should be a field of that class rather than a field of CoarseGrainedSchedulerBackend.

[SparkQA]

Test build #33348 has finished for PR 6323 at commit 12d8c9e.

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

[JoshRosen]

LGTM, so I'm going to merge this to master. Although I think this is probably safe for 1.4, I don't want to risk conflicting with any attempts to cut another RC tonight, so I'm not going to pick it there for now. Thanks!