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

[abhi951990]

What changes were proposed in this pull request?

Removed the DeadCode as suggested.

[srowen]

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?

[andrewor14]

@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?

[davies]

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);
    }

[andrewor14]

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

[HyukjinKwon]

@abhi951990 I think maybe it would be nicer if the title is more specific just like the title of this JIRA ticket. I think it would be even nicer if the title follows the format like others such as [SPARK-15045][CORE] XXXX (See https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark)

[andrewor14]

ok to test

[SparkQA]

Test build #57700 has finished for PR 12829 at commit 06be5e9.

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

[andrewor14]

@abhi951990 I don't think this is dead code. It seems to be a potential bug.

[abhi951990]

@andrewor14 So should I only delete line 382? Or should I leave as it is?

[andrewor14]

Let's see what @davies thinks.

[SparkQA]

Test build #57835 has finished for PR 12829 at commit ad748dd.

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

[davies]

LGTM, merging into master, thanks!