SPARK-12729. PhantomReference to replace finalize in python broadcast…

[GayathriMurali]

What changes were proposed in this pull request?

Replace finalize() method in PythonBroadcast with Phantom Reference. Redesigned some portions of the code to better handle the thread to avoid thread leak. Fixed existing null pointer exceptions. Introduced a new thread class and a phantom reference class.

How was the this patch tested?

build/sbt "test-only org.apache.spark.api.python.*" - Passes all tests

[GayathriMurali]

@davies @zsxwing Can you please verify this patch?

[SparkQA]

Test build #2547 has finished for PR 11257 at commit f0dc402.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[zsxwing]

who calls this method?

[GayathriMurali]

@zsxwing Most documentations on phantom reference suggests a separate daemon thread to do the cleanup. I can try adding the phantom reference removal from the queue in the same function without a thread. Would that work?

[zsxwing]

@zsxwing Most documentations on phantom reference suggests a separate daemon thread to do the cleanup. I can try adding the phantom reference removal from the queue in the same function without a thread. Would that work?

The problem is it will create one thread for each PythonBroadcast. If there are hundreds of PythonBroadcasts alive, it will create hundreds of threads.

[GayathriMurali]

@zsxwing how about creating one thread and the reference queue would contain all instances of the file object that needs to be GC'ed? Start a global thread?

[GayathriMurali]

@zsxwing Should i modify the code to use one thread?

[zsxwing]

@GayathriMurali actually, I'm not sure if it will work. E.g., you need to create a global thread, but when to stop that?

[GayathriMurali]

@zsxwing when the queue is empty? Add a listener to the queue to invoke or stop the thread accordingly? Would this approach still make Phantom reference more beneficial than finalize()?

[zsxwing]

@zsxwing when the queue is empty? Add a listener to the queue to invoke or stop the thread accordingly? Would this approach still make Phantom reference more beneficial than finalize()?

Right. I'm thinking about it. Actually, it looks much more complicated than it was thought. Maybe let's just keep it unchanged until someone complaints the bad performance.

[srowen]

Why not use a finalizer? this is looking pretty complex with new threads and reference schemes otherwise

[GayathriMurali]

@srowen The JIRA was created by @davies and the intent was to replace finalize() with Phantom Reference. http://resources.ej-technologies.com/jprofiler/help/doc/index.html

[srowen]

I think this is the wrong solution. finalize() should not do significant work unless something went wrong -- some expensive resource was not closed. Is that happening regularly? then I think that call site needs to be fixed. This pushes the problem somewhere else.

[davies]

The finalize() here is to cleanup the disk file for Python broadcasts, is the regular path.

@srowen This change is requested by @rxin, to avoid blocking operations in finalize().

Since the Python broadcasts are rarely used and exists() and delete() should be lightly system calls, I'm fine with current finalizer.

[srowen]

Another way to avoid blocking in finalize is to not do the work in the finalizer. It's as simple as making an idempotent "close" or "cleanup" method that does nothing if it has already been disposed. ... are these broadcasts not otherwise cleaned up?

To the extent they're rarely used, is this a problem?

My concern is that you're simply reimplementing a separate 'finalizer' queue with all the associated complexity.

[rxin]

The problem is blocking GC threads ...

[srowen]

Yes, I certainly understand that. finalize shouldn't do non-trivial work. Objects shouldn't require non-trivial cleanup, in general, when they hit the finalizer. This is typically a symptom of not cleaning up the object in normal execution paths. Why is that not possible?

[rxin]

Yea we could do that too. Actually explicit management is probably better, with a fallback to do implicit management so it is robust against mem leaks. That is provided somebody has the cycles to do it.

[rxin]

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.