Skip to content

[SPARK-15045] [CORE] Remove dead code in TaskMemoryManager.cleanUpAllAllocatedMemory for pageTable #12829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abhi951990 wants to merge 2 commits into from
Closed

[SPARK-15045] [CORE] Remove dead code in TaskMemoryManager.cleanUpAllAllocatedMemory for pageTable #12829

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06be5e9
Update TaskMemoryManager.java
abhi951990 May 2, 2016
ad748dd
Updated the new patch by rewriting the synchronized block
abhi951990 May 5, 2016
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
13 changes: 7 additions & 6 deletions core/src/main/java/org/apache/spark/memory/TaskMemoryManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,22 +379,23 @@ public long getOffsetInPage(long pagePlusOffsetAddress) {
*/
public long cleanUpAllAllocatedMemory() {
synchronized (this) {
Arrays.fill(pageTable, null);
for (MemoryConsumer c: consumers) {
if (c != null && c.getUsed() > 0) {
// In case of failed task, it's normal to see leaked memory
logger.warn("leak " + Utils.bytesToString(c.getUsed()) + " memory from " + c);
}
}
consumers.clear();
}

for (MemoryBlock page : pageTable) {
if (page != null) {
memoryManager.tungstenMemoryAllocator().free(page);
for (MemoryBlock page : pageTable) {
if (page != null) {
logger.warn("leak a page: " + page + " in task " + taskAttemptId);
Copy link
Member

@srowen srowen May 2, 2016

Choose a reason for hiding this comment

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

It's not clear this code should be removed, though I agree right now it appears to be dead code because of line 382. CC @JoshRosen since I think you added this block? should line 382 just be removed?

Copy link
Contributor

@andrewor14 andrewor14 May 4, 2016

Choose a reason for hiding this comment

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

@davies in L382 we null out the pageTable so this loop doesn't do anything. However, I don't think we should delete this code. Should we just remove L382? Is this a cause for memory leaks?

Copy link
Contributor

@davies davies May 4, 2016

Choose a reason for hiding this comment

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

Chatted with @JoshRosen , these lines are introduce by logical conflict, it could be rewritten as

    synchronized (this) {
      for (MemoryConsumer c: consumers) {
        if (c != null && c.getUsed() > 0) {
          // In case of failed task, it's normal to see leaked memory
          logger.warn("leak " + Utils.bytesToString(c.getUsed()) + " memory from " + c);
        }
      }
      consumers.clear();

      for (MemoryBlock page : pageTable) {
        if (page != null) {
          logger.warn("leak a page: " + page + " in task " + taskAttemptId);
          memoryManager.tungstenMemoryAllocator().free(page);
        }
      }
      Arrays.fill(pageTable, null);
    }

Copy link
Contributor

@andrewor14 andrewor14 May 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

that LGTM. @abhi951990 can you update this patch to reflect that?

memoryManager.tungstenMemoryAllocator().free(page);
}
}
Arrays.fill(pageTable, null);
}
Arrays.fill(pageTable, null);


// release the memory that is not used by any consumer.
memoryManager.releaseExecutionMemory(acquiredButNotUsed, taskAttemptId, tungstenMemoryMode);
Expand Down