Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -573,19 +573,29 @@ public boolean hasNext() {

@Override
public void loadNext() throws IOException {
synchronized (this) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible lastPage != null on line 550 above when it is set? https://github.com/apache/spark/pull/24269/files#diff-027299fb14327ddcaba457f81ecff32cR550

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible but doesn't matter. It's kind of we delay the page releasing a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that both the 2 methods are inside synchronized (this)

loaded = true;
if (nextUpstream != null) {
// Just consumed the last record from in memory iterator
if (lastPage != null) {
freePage(lastPage);
lastPage = null;
MemoryBlock pageToFree = null;
try {
synchronized (this) {
loaded = true;
if (nextUpstream != null) {
// Just consumed the last record from in memory iterator
if (lastPage != null) {
// Do not free the page here, while we are locking `SpillableIterator`. The `freePage`
// method locks the `TaskMemoryManager`, and it's a bad idea to lock 2 objects in
// sequence. We may hit dead lock if another thread locks `TaskMemoryManager` and
// `SpillableIterator` in sequence, which may happen in
// `TaskMemoryManager.acquireExecutionMemory`.
pageToFree = lastPage;
lastPage = null;
}
upstream = nextUpstream;
nextUpstream = null;
}
upstream = nextUpstream;
nextUpstream = null;
numRecords--;
upstream.loadNext();
}
numRecords--;
upstream.loadNext();
} finally {
if (pageToFree != null) freePage(pageToFree);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add braces (according to Oracle java style: if statements always use braces, {})

}
}

Expand Down