[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

viirya · 2018-12-12T04:06:31Z

What changes were proposed in this pull request?

Based on the comment, it seems to be better to put freePage into a finally block. This patch as a follow-up to do so.

How was this patch tested?

Existing tests.

viirya · 2018-12-12T04:06:43Z

cc @cloud-fan @kiszk

viirya · 2018-12-12T04:37:28Z

@cloud-fan do we also need a similar followup for branch-2.4?

cloud-fan · 2018-12-12T05:29:05Z

I think this can be merged to 2.4 without conflict. I'll ping you if it doesn't. Thanks!

kiszk · 2018-12-12T07:09:57Z

LGTM, thanks

SparkQA · 2018-12-12T08:05:02Z

Test build #100002 has finished for PR 23294 at commit 630281b.

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

kiszk · 2018-12-12T08:12:19Z

retest this please

SparkQA · 2018-12-12T12:34:28Z

Test build #100011 has finished for PR 23294 at commit 630281b.

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

SparkQA · 2018-12-12T12:35:27Z

Test build #100013 has finished for PR 23294 at commit 630281b.

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

HyukjinKwon · 2018-12-12T12:55:11Z

Argh, again!

* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]
Execution halted

@felixcheung, @viirya

viirya · 2018-12-12T13:35:07Z

@HyukjinKwon Thanks for letting me know. I will look it and ask CRAN admin for help.

viirya · 2018-12-12T14:21:06Z

retest this please.

viirya · 2018-12-12T14:29:26Z

@HyukjinKwon I don't find any problem locally. Let me re-trigger the Jenkins test and confirm it.

SparkQA · 2018-12-12T18:56:26Z

Test build #100026 has finished for PR 23294 at commit 630281b.

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

viirya · 2018-12-12T23:56:03Z

Sorry, I found out where the problem is. Already asked CRAN admin for help.

HyukjinKwon · 2018-12-13T00:18:49Z

Thanks, @viirya.

HyukjinKwon · 2018-12-13T00:19:31Z

Hey, @viirya, can you send an email to dev mailing list where we're discussing about this, when the tests get back to normal please?

viirya · 2018-12-13T00:26:15Z

@HyukjinKwon Ok. But I think we better discuss this on the related JIRA ticket SPARK-24152.

HyukjinKwon · 2018-12-13T00:30:13Z

Yea that sounds more appropriate place to discuss. As a bonus, looks people looks getting confused why it's being failed.. Let's send new email as new thread that the tests became normal as well then.

HyukjinKwon · 2018-12-13T02:38:45Z

retest this please

viirya · 2018-12-13T07:16:12Z

retest this please.

SparkQA · 2018-12-13T07:26:54Z

Test build #100061 has finished for PR 23294 at commit 630281b.

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

felixcheung · 2018-12-13T07:51:19Z

all pass

cloud-fan · 2018-12-13T07:59:50Z

core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java

          }
-          try {
-            Closeables.close(reader, /* swallowIOException = */ false);


shall we close the reader outside of the synchronized block? Then we don't need to extra try catch.

In the second try, it accesses spillWriters:

reader = spillWriters.getFirst().getReader(serializerManager)

The synchronized spill will change spillWriters. Seems to be unsafe if we close the reader and update reader like above outside synchronized?

how about

UnsafeSorterSpillReader readerToClose = null; synchronized (this) { ... readerToClose = reader; reader = spillWriters.getFirst().getReader(serializerManager); ... } try { Closeables.close(readerToClose, /* swallowIOException = */ false); } catch ...

I'm not sure of the semantics here, but because the close block is changing several fields at once, it seems more conservative to leave them as they are within the synchronized block. At least, that is a separate issue.

@cloud-fan What do you think?

I think it's okay to go ahead as is

SparkQA · 2018-12-13T08:05:01Z

Test build #100074 has finished for PR 23294 at commit 630281b.

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

SparkQA · 2018-12-14T17:28:05Z

Test build #4468 has finished for PR 23294 at commit 630281b.

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

SparkQA · 2018-12-15T00:02:46Z

Test build #4471 has finished for PR 23294 at commit 630281b.

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

HyukjinKwon · 2018-12-15T03:55:25Z

retest this please

HyukjinKwon · 2018-12-15T05:52:03Z

tests were already passed for the same commit at #23294 (comment)

Merged to master and branch-2.4.

## What changes were proposed in this pull request? Based on the [comment](#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes #23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 1b604c1) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

SparkQA · 2018-12-15T08:05:02Z

Test build #100174 has finished for PR 23294 at commit 630281b.

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

## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

## What changes were proposed in this pull request? Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. ## How was this patch tested? Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 1b604c1) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…locking both BytesToBytesMap.MapIterator and TaskMemoryManager In `BytesToBytesMap.MapIterator.advanceToNextPage`, We will first lock this `MapIterator` and then `TaskMemoryManager` when going to free a memory page by calling `freePage`. At the same time, it is possibly that another memory consumer first locks `TaskMemoryManager` and then this `MapIterator` when it acquires memory and causes spilling on this `MapIterator`. So it ends with the `MapIterator` object holds lock to the `MapIterator` object and waits for lock on `TaskMemoryManager`, and the other consumer holds lock to `TaskMemoryManager` and waits for lock on the `MapIterator` object. To avoid deadlock here, this patch proposes to keep reference to the page to free and free it after releasing the lock of `MapIterator`. Added test and manually test by running the test 100 times to make sure there is no deadlock. Closes apache#23272 from viirya/SPARK-26265. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry-picked from commit a3bbca9) [SPARK-26265][CORE][FOLLOWUP] Put freePage into a finally block Based on the [comment](apache#23272 (comment)), it seems to be better to put `freePage` into a `finally` block. This patch as a follow-up to do so. Existing tests. Closes apache#23294 from viirya/SPARK-26265-followup. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry-picked from commit 1b604c1) Ref: LIHADOOP-43221 RB=1518143 BUG=LIHADOOP-43221 G=superfriends-reviewers R=fli,mshen,yezhou,edlu A=fli

Put freePage into a finally.

630281b

cloud-fan approved these changes Dec 12, 2018

View reviewed changes

dongjoon-hyun approved these changes Dec 12, 2018

View reviewed changes

HyukjinKwon approved these changes Dec 12, 2018

View reviewed changes

felixcheung approved these changes Dec 12, 2018

View reviewed changes

dongjoon-hyun mentioned this pull request Dec 12, 2018

[SPARK-26203][SQL][TEST] Benchmark performance of In and InSet expressions #23291

Closed

cloud-fan reviewed Dec 13, 2018

View reviewed changes

asfgit closed this in 1b604c1 Dec 15, 2018

viirya deleted the SPARK-26265-followup branch December 27, 2023 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

cloud-fan commented Dec 12, 2018

kiszk commented Dec 12, 2018

SparkQA commented Dec 12, 2018

kiszk commented Dec 12, 2018

SparkQA commented Dec 12, 2018

SparkQA commented Dec 12, 2018

HyukjinKwon commented Dec 12, 2018

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

SparkQA commented Dec 12, 2018

viirya commented Dec 12, 2018

HyukjinKwon commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

viirya commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

viirya commented Dec 13, 2018

SparkQA commented Dec 13, 2018

felixcheung commented Dec 13, 2018

cloud-fan Dec 13, 2018

viirya Dec 13, 2018

cloud-fan Dec 13, 2018

srowen Dec 13, 2018

viirya Dec 14, 2018

HyukjinKwon Dec 14, 2018

SparkQA commented Dec 13, 2018

SparkQA commented Dec 14, 2018

SparkQA commented Dec 15, 2018

HyukjinKwon commented Dec 15, 2018

HyukjinKwon commented Dec 15, 2018

SparkQA commented Dec 15, 2018

[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

[SPARK-26265][Core][Followup] Put freePage into a finally block #23294

Conversation

viirya commented Dec 12, 2018

What changes were proposed in this pull request?

How was this patch tested?

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

cloud-fan commented Dec 12, 2018

kiszk commented Dec 12, 2018

SparkQA commented Dec 12, 2018

kiszk commented Dec 12, 2018

SparkQA commented Dec 12, 2018

SparkQA commented Dec 12, 2018

HyukjinKwon commented Dec 12, 2018

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

viirya commented Dec 12, 2018

SparkQA commented Dec 12, 2018

viirya commented Dec 12, 2018

HyukjinKwon commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

viirya commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

HyukjinKwon commented Dec 13, 2018

viirya commented Dec 13, 2018

SparkQA commented Dec 13, 2018

felixcheung commented Dec 13, 2018

cloud-fan Dec 13, 2018

Choose a reason for hiding this comment

viirya Dec 13, 2018

Choose a reason for hiding this comment

cloud-fan Dec 13, 2018

Choose a reason for hiding this comment

srowen Dec 13, 2018

Choose a reason for hiding this comment

viirya Dec 14, 2018

Choose a reason for hiding this comment

HyukjinKwon Dec 14, 2018

Choose a reason for hiding this comment

SparkQA commented Dec 13, 2018

SparkQA commented Dec 14, 2018

SparkQA commented Dec 15, 2018

HyukjinKwon commented Dec 15, 2018

HyukjinKwon commented Dec 15, 2018

SparkQA commented Dec 15, 2018