[SPARK-3116] Remove the excessive lockings in TorrentBroadcast

[rxin]

No description provided.

[rxin]

cc @mosharaf @shivaram

[SparkQA]

QA tests have started for PR 2028 at commit 92c62a5.

• This patch merges cleanly.

[shivaram]

Can you clarify what is the guarantee that we are trying to maintain in this class ? Is it that if two executor threads try to read the same broadcast variable we want it to be safe ?

[rxin]

Yes.

[shivaram]

LGTM.Alright I looked through the code and tried to verify this change was safe. Just summarizing what I saw while walking through the code (in case it helps in the future)

1. The first synchronized is in the constructor which two threads cannot call at the same time
2. sendBroadcast is only called from constructor, so that is safe to remove
3. receiveBroadcast is only called from a synchronized block in readObject, so that should be safe to remove (however this should be free in java, to re-acquire an already held lock)
4. unpersist doesn't touch the class, so it is thread-safe (assuming BlockManager's readBroadcast is thread-safe -- I didn't check that)

[SparkQA]

QA tests have finished for PR 2028 at commit 92c62a5.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class Params(input: String = "data/mllib/sample_linear_regression_data.txt")
  • case class Params(input: String = "data/mllib/sample_linear_regression_data.txt")
  • case class Params(input: String = "data/mllib/sample_binary_classification_data.txt")

[rxin]

Ok I'm merging this in master & branch-1.1.

[JoshRosen]

The first synchronized is in the constructor which two threads cannot call at the same time

Actually, I think two threads could get into the constructor simultaneously, but it wouldn't be a problem because the calls would be for different broadcast ids. ( new TorrentBroadcast is only called from TorrentBroadcastFactory.newBroadcast, which is only called from BroadcastManager.newBroadcast. This, in turn, is only called from SparkContext.broadcast, which isn't synchronized.)

[shivaram]

Hmm -- according to JLS [1], objects are only made available to other threads after the constructor finishes. So technically only one thread can call an object's constructor. However constructors are not thread safe if we expose a reference to this to other classes / threads [2].

In this case I didn't see the reference leaking, but I guess it doesn't hurt to track down the callers to verify things.

[1] http://docs.oracle.com/javase/specs/jls/se5.0/html/classes.html#8.8.3
[2] http://stackoverflow.com/questions/4880168/why-cant-java-constructors-be-synchronized

[JoshRosen]

Regarding the first synchronized in that constructor, my understanding is that the original TorrentBroadcast.synchronized was guarding the writing of the broadcast value to the BlockManager. I think that this synchronization is unnecessary not because we don't leak this, but because BlockManager is thread-safe and the uniqueness of broadcast ids means that we'll never have parallel attempts to write the same block.

My point was that multiple TorrentBroadcast objects can be simultaneously under construction, but that it's safe because they'll have distinct ids; I think preventing leakage of a partially-constructed object is a separate issue that doesn't come into play here.