[SPARK-21319][SQL] Fix memory leak in sorter #18679

cloud-fan · 2017-07-19T08:30:19Z

What changes were proposed in this pull request?

UnsafeExternalSorter.recordComparator can be either KVComparator or RowComparator, and both of them will keep the reference to the input rows they compared last time.

After sorting, we return the sorted iterator to upstream operators. However, the upstream operators may take a while to consume up the sorted iterator, and UnsafeExternalSorter is registered to TaskContext at here, which means we will keep the UnsafeExternalSorter instance and keep the last compared input rows in memory until the sorted iterator is consumed up.

Things get worse if we sort within partitions of a dataset and coalesce all partitions into one, as we will keep a lot of input rows in memory and the time to consume up all the sorted iterators is long.

This PR takes over #18543 , the idea is that, we do not keep the record comparator instance in UnsafeExternalSorter, but a generator of record comparator.

close #18543

How was this patch tested?

N/A

cloud-fan · 2017-07-19T08:31:02Z

This bug was originally reported by @j-baker

also cc @kiszk and @srowen to review.

srowen · 2017-07-19T08:52:56Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeInMemorySorter.java

@Override?

Since SortComparator does not extend RecordComparator, @Override may not be necessary.

kiszk · 2017-07-19T11:05:24Z

LGTM

j-baker · 2017-07-19T11:29:03Z

I'm not quite convinced this works in the way we want if spilling occurs.

j-baker · 2017-07-19T11:32:00Z

I would propose copying the comparator before giving it to anything that might use this, so that we never hold onto a comparator that someone might have left dirty.

SparkQA · 2017-07-19T11:32:06Z

Test build #79746 has finished for PR 18679 at commit f5752d4.

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

cloud-fan · 2017-07-19T12:46:52Z

Since this PR definitely fixes the non-spilling case, I'm going to merge it and @j-baker can continue his work to fix the spilling case(if it's problematic).

j-baker · 2017-07-19T12:47:06Z

But the spilling case is the issue?

j-baker · 2017-07-19T12:47:24Z

I think that #18543 (post rewrite) is a better approach than this - instead of retaining a memory leak, we remove the opportunity for one to appear.

cloud-fan · 2017-07-19T12:50:20Z

I'm not convinced spilling case is the issue, can you verify that after this PR, your workload still OOM?

j-baker · 2017-07-19T12:52:52Z

So in my build, I definitely have spilling (quite a lot of it). If I have spilling, then the last thing that uses the comparator will definitely be the merge sorter. The last thing that uses the comparator is the thing that causes the memory leak.

j-baker · 2017-07-19T13:03:06Z

Oh, or are you saying that because in the external case most of the buffers stay on disk, you wouldn't see this issue because there's not enough stuff in memory? That'd make sense.

This seems fine to merge - not sure if you prefer the approach in my PR (make sure the comparator exists only in ephemeral objects).

cloud-fan · 2017-07-19T13:13:18Z

I left a comment in #18543 (comment)

your concern is valid, in the case of coalesce, we may have many mergers in one task, and some mergers may finish earlier and waiting for the end of the task to do cleanup.

There are 3 possible fixes:

do cleanup at the end of the in-memory sorter and the merger. Like this PR did
copy the comparator when passing to the in-memory sorter and the merger. Like [SPARK-21319][SQL] Fix memory leak in UnsafeExternalRowSorter.RowComparator #18543 did
avoid referring UnsafeExternalSorter in TaskContext.

cloud-fan · 2017-07-19T13:14:25Z

cc @hvanhovell

cloud-fan · 2017-07-19T13:16:51Z

~~Personally I prefer option 3 if it's doable.~~

3 is not doable, this PR goes with option 2.

cloud-fan · 2017-07-26T17:57:23Z

cc @ueshin

SparkQA · 2017-07-26T19:56:22Z

Test build #79971 has finished for PR 18679 at commit 9b88297.

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

ueshin

LGTM except for 1 comment.

ueshin · 2017-07-27T04:32:49Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

-        this, taskMemoryManager, recordComparator, prefixComparator, initialSize, canUseRadixSort);
+        this,
+        taskMemoryManager,
+        recordComparatorSupplier.get(),


Seems like we need to null check for recordComparatorSupplier.

SparkQA · 2017-07-27T10:23:50Z

Test build #79994 has finished for PR 18679 at commit 4903670.

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

cloud-fan · 2017-07-27T10:32:39Z

retest this please

hvanhovell · 2017-07-27T11:19:31Z

LGTM - pending jenkins

SparkQA · 2017-07-27T13:29:51Z

Test build #80000 has finished for PR 18679 at commit 4903670.

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

cloud-fan · 2017-07-27T15:02:00Z

thanks for the review, merging to master!

srowen reviewed Jul 19, 2017

View reviewed changes

Fix memory leak in external sorter

9b88297

cloud-fan force-pushed the memory-leak branch from f5752d4 to 9b88297 Compare July 26, 2017 17:45

ueshin reviewed Jul 27, 2017

View reviewed changes

address comment

4903670

asfgit closed this in 9f5647d Jul 27, 2017

ash211 mentioned this pull request Aug 21, 2017

[NOSQUASH] Resync from Apache palantir/spark#247

Merged

[SPARK-21319][SQL] Fix memory leak in sorter #18679

[SPARK-21319][SQL] Fix memory leak in sorter #18679

Uh oh!

Conversation

cloud-fan commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Jul 19, 2017

Uh oh!

srowen Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

kiszk Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

kiszk commented Jul 19, 2017

Uh oh!

j-baker commented Jul 19, 2017

Uh oh!

j-baker commented Jul 19, 2017

Uh oh!

SparkQA commented Jul 19, 2017

Uh oh!

cloud-fan commented Jul 19, 2017

Uh oh!

j-baker commented Jul 19, 2017

Uh oh!

j-baker commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jul 19, 2017

Uh oh!

j-baker commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-baker commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jul 19, 2017

Uh oh!

cloud-fan commented Jul 19, 2017

Uh oh!

cloud-fan commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jul 26, 2017

Uh oh!

SparkQA commented Jul 26, 2017

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2017

Uh oh!

cloud-fan commented Jul 27, 2017

Uh oh!

hvanhovell commented Jul 27, 2017

Uh oh!

SparkQA commented Jul 27, 2017

Uh oh!

cloud-fan commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cloud-fan commented Jul 19, 2017 •

edited

Loading

j-baker commented Jul 19, 2017 •

edited

Loading

j-baker commented Jul 19, 2017 •

edited

Loading

j-baker commented Jul 19, 2017 •

edited

Loading

cloud-fan commented Jul 19, 2017 •

edited

Loading