Remove internal lazy val from Later. #989

non · 2016-04-20T05:28:02Z

This commit replaces the previous Later implementation
with one that is not based on Scala's lazy val encoding.

Concretely, this new implementation uses a private,
internal lock to avoid potential deadlocks that could
arise if other objects synchronized on a Later instance.
The possibility of this happening is relatively remote
but not impossible.

Performance seems to be equivalent to lazy vals in the
cases that were tested. An additional Object is
allocated, so Later instance may be slightly larger.

Is this a good idea? Should we stick with lazy val?
Should we synchronize on this to have smaller
instances? Should we be doing something else?

This commit replaces the previous Later implementation with one that is not based on Scala's lazy val encoding. Concretely, this new implementation uses a private, internal lock to avoid potential deadlocks that could arise if other objects synchronized on a Later instance. The possibility of this happening is relatively remote but not impossible. Performance seems to be equivalent to lazy vals in the cases that were tested. An additional Object is allocated, so Later instance may be slightly larger.

ceedubs · 2016-04-20T13:10:05Z

core/src/main/scala/cats/Eval.scala

+    if (thunk == null) {
+      result
+    } else lock.synchronized {
+      if (thunk == null) result else {


I think that double-checked locking on the JVM requires a volatile var because of...reasons. http://stackoverflow.com/questions/7855700/why-is-volatile-used-in-this-example-of-double-checked-locking hits on this. You are definitely more of a JVM expert than I am though, so I feel like you could probably convince me that this isn't a concern.

So I think the reason this isn't a problem is because we are only setting thunk = null after result = thunk() has fully initialized the result: A value. This means that we don't have to worry about someone else returning a partially-evaluated result value. I could be wrong though. I looked at this code with @johnynek -- maybe he can weigh in here too?

ceedubs · 2016-04-20T13:19:19Z

I imagine that this might have been inspired in part by scalaz/scalaz#1145 ? If so, I think that the lazy val in Later is significantly less of a concern than the places addressed in that PR (which was inspired by some profiling we had done at work). In that case, lazy vals were being used for evaluate-once semantics on method-level values, causing the entire object to be locked for something that really only needed to apply to a particular call of a particular method. For example, a call to Apply[Option].apply7 would break down into a bunch of ap calls on the (singleton) Apply[Option] instance, so the lazy vals in those ap methods would end up hitting a lot of contention on the locking of the Apply[Option] instance.

In the case of Later, I think object-level locking is roughly the desired execution.

It's quite possible that this is still a good idea, and I definitely think you would be a better judge of that than I. I just wanted to bring to attention the difference between Later and the changes made in scalaz.

codecov-io · 2016-04-20T13:22:05Z

Current coverage is `90.73%`

Merging #989 into master will decrease coverage by -0.09% as of e7facba

@@            master    #989   diff @@
======================================
  Files          184     184       
  Stmts         2181    2202    +21
  Branches        43      46     +3
  Methods          0       0       
======================================
+ Hit           1981    1998    +17
  Partial          0       0       
- Missed         200     204     +4

Review entire Coverage Diff as of e7facba

Powered by Codecov. Updated on successful CI builds.

non · 2016-04-20T14:49:06Z

@ceedubs I would be comfortable moving the locking to this if everyone agrees that locking Later is acceptable and are not worried about cases where a user haplessly does their own locking. I'd rather remove lazy val in any case since we can do the encoding more directly.

xuwei-k · 2016-04-20T15:39:58Z

https://gist.github.com/xuwei-k/4be690a606bec83683563cfff525b3a5

ceedubs · 2016-04-20T16:18:19Z

@xuwei-k wow, super helpful. Thanks!

non · 2016-04-20T16:42:02Z

Thanks!

johnynek · 2016-04-20T17:15:47Z

I think if you check result != null this works.

On Wednesday, April 20, 2016, Erik Osheim notifications@github.com wrote:

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#989 (comment)

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

johnynek · 2016-04-20T17:19:53Z

Actually result needs to volatile. See this paper:

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

The solution at the end hat works with Java 5 (I think we can skip Java 4
support).

On Wednesday, April 20, 2016, P. Oscar Boykin oscar.boykin@gmail.com
wrote:

I think if you check result != null this works.

On Wednesday, April 20, 2016, Erik Osheim <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#989 (comment)

P. Oscar Boykin, Ph.D. | http://twitter.com/posco |
http://pobox.com/~boykin

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

johnynek · 2016-04-20T19:19:33Z

Just to be clear, locking this can cause a deadlock. I don't see how the correct volatile solution has that risk. The risk is small (some external caller has to lock the Eval[T] instance and another object while the Lazy locks that same object), but I'd prefer it to be zero.

This commit also adds a synchronization test, based on Kenji's example. I confirmed that this test fails when the @volatile annotation is removed from thunk.

non · 2016-04-28T16:31:24Z

Thanks for the example @xuwei-k! I added this as a test case.

For now, the easiest way to test synchronization is to just let some threads run for awhile. But that makes the current test difficult to write. Eventually we should think about a good way to test this on JS but for now I think a JVM-only test is OK.

ceedubs · 2016-05-02T21:44:16Z

@non I see that you have gone to some length to ensure that this is serializable. Is that being checked by a test, or should we be adding one?

It's been mentioned by both @non and @johnynek that the chances of deadlock from locking on this are pretty remote. Considering that it's quite remote and people are kind of saying that they know what they are doing if they start locking on the instance, I'd be inclined to lock on this and avoid the extra Object allocation (which lingers as long as the Eval instance exists, increasing its memory footprint as far as I can tell). However, @non and @johnynek are much more performance-conscious than I am and seem to like this approach, so 👍 by me.

johnynek · 2016-05-03T00:24:44Z

the serialization was exercised by the semigroup serialization check (indeed that is why we added the code).

I hear you that it is pretty defensive to worry about locks on this. I don't mind locking this I guess. I would never lock an object I didn't create since I have no idea who else might lock it, so I guess it is clearly dangerous, and this is scala, so we assume programmers know about sharp edges and avoid them.

mikejcurry · 2016-05-03T06:50:44Z

core/src/main/scala/cats/Eval.scala

+
+  final def value: A =
+    if (thunk == null) {
+      result


I think there might be a nasty corner case here where both thunk and result evaluate to null from a secondary threads perspective, but would definitely love to find out I'm wrong.
Consider:

Thread A enters value method and inspects thunk, seeing thunk as non null

Thread A enters synchronized block

Thread A inspects thunk again, and sees it as non null

Thread A enters else branch and evaluates thunk, assigning the calculated A to result

Thread A assigns null to thunk

Thread B now enters value method and inspects thunk

Thread B sees thunk as null due to volatility. The value has been written in Thread A as null.

Thread B returns the value of result

I think that it is possible for the Thread B result variable to still point to a null value, briefly in this scenario because I don't think either the volatility of thunk or the synchronized block which this code block does not take place in, make any guarantees about which value Thread B might see as the value in result. If I'm wrong here it would be great to find out why as it would improve my understanding of the JVM.

yeah, I think that's a real issue. I think switching the volatile to
result and checking that would be safe, but requires a cast since A >: Null is not guaranteed (but should be safe on the jvm).

On Mon, May 2, 2016 at 8:50 PM, Mike Curry notifications@github.com wrote:

In core/src/main/scala/cats/Eval.scala
#989 (comment):

lazy val value: A = {

val result = thunk()

thunk = null // scalastyle:off

result

}
+final class Later[A](f: %28%29 => A) extends Eval[A] with Externalizable {

// we use lock to prevent possible problems with someone else

// synchronizing on 'this'.

private[this] val lock = new Object()

@volatile private[this] var thunk: Function0[A] = f

private[this] var result: A = _

final def value: A =

if (thunk == null) {

result

I think there might be a nasty corner case here where both thunk and
result evaluate to null from a secondary threads perspective, but would
definitely love to find out I'm wrong.
Consider:

Thread A enters value method and inspects thunk, seeing thunk as non
null

Thread A enters synchronized block

Thread A inspects thunk again, and sees it as non null

Thread A enters else branch and evaluates thunk, assigning the
calculated A to result

Thread A assigns null to thunk

Thread B now enters value method and inspects thunk

Thread B sees thunk as null due to volatility. The value has been
written in Thread A as null.

Thread B returns the value of result

I think that it is possible for the Thread B result variable to still
point to a null value, briefly in this scenario because I don't think
either the volatility of thunk or the synchronized block which this code
block does not take place in, make any guarantees about which value Thread
B might see as the value in result. If I'm wrong here it would be great
to find out why as it would improve my understanding of the JVM.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/typelevel/cats/pull/989/files/39828830bb958ff9bc775ed34897e72d3f59a7c6#r61843443

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

Yea, agree - switching the volatile to result should close any loops. I don't think you need a cast though - at least not an explicit one - this should be fine.

final class Later[A](f: () => A) extends Eval[A] { // we use lock to prevent possible problems with someone else // synchronizing on 'this'. private[this] val lock = new Object() private[this] var thunk: Function0[A] = f @volatile private[this] var result: A = _ final def value: A = if (result != null) { result } else lock.synchronized { if (result != null) result else { result = thunk() thunk = null result } } def memoize: Eval[A] = this }

BTW - here is an alternate encoding. Not sure what would be better - but it is an interesting problem so I was thinking of a few different approaches and this one seemed interesting.

final class Later[A](f: () => A) extends Eval[A] { private[this] var thunk: () => A = f private[this] trait Result { def value: A } private[this] val evaluated: Result = new Result { lazy val value = { val result = thunk() thunk = null result } } final def value: A = evaluated.value def memoize: Eval[A] = this }

This encoding avoids explicit use of volatile or synchronized, and moves the lock to an inner type so should avoid the synchronization problem mentioned. I'm not sure how the cost of the additional reference to the lazy valin the inner type would affect performance, I guess it would need to be measured.

I guess in the case where we check result != null we also need to throw an NPE or something if thunk() == null.

I'm not sure I understand.thunk() == null should never be true, as it is the evaluation of a Function0[A]- which evaluates to an A. Well - at least no more than any other scala function with a return type of A can evaluate to null. So, I think thunk() == null should not be considered here.

The reference to thunk can equal null, i.e. thunk == null, but there is only one dereference of thunk to and this is in the synchronized block, which is only entered when result == null.

So, I think that the volatile nature of result would make that safe.

I agree one should not return null, but if they do, this code will first
return null, but on a subsequent call would NPE (you would try to call
thunk again, but it would be null).

To me, this behavior sounds no good. I'd rather NPE the first time around
hen returning null once then failing if you happen to get the value again.

On Wednesday, May 4, 2016, Mike Curry notifications@github.com wrote:

In core/src/main/scala/cats/Eval.scala
#989 (comment):

lazy val value: A = {

val result = thunk()

thunk = null // scalastyle:off

result

}
+final class Later[A](f: %28%29 => A) extends Eval[A] with Externalizable {

// we use lock to prevent possible problems with someone else

// synchronizing on 'this'.

private[this] val lock = new Object()

@volatile private[this] var thunk: Function0[A] = f

private[this] var result: A = _

final def value: A =

if (thunk == null) {

result

I'm not sure I understand.thunk() == null should never be true, as it is
the evaluation of a Function0[A]- which evaluates to an A. Well - at
least no more than any other scala function with a return type of A can
evaluate to null. So, I think thunk() == null should not be considered
here.

The reference to thunk can equal null, i.e. thunk == null, but there is
only one dereference of thunk to and this is in the synchronized block,
which is only entered when result == null.

So, I think that the volatile nature of result would make that safe.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/typelevel/cats/pull/989/files/39828830bb958ff9bc775ed34897e72d3f59a7c6#r62152850

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

I agree. That would be nasty.

non · 2016-05-30T19:29:40Z

I'm going to rescind this for now, until @johnynek and I have a chance to think about this more and do more performance testing. I think the issue it's solving (what if someone locks a Later instance) are unlikely to be a problem for most people (especially compared to the "what if someone locks their own class that has a lazy val" problem).

non added the in progress label Apr 20, 2016

non force-pushed the topic/no-lazy-val-eval branch from ac22d9e to dd3e299 Compare April 20, 2016 12:33

erik-stripe added 2 commits April 20, 2016 08:39

Fix style error.

a9e7af9

Fix more style errors.

219a660

ceedubs reviewed Apr 20, 2016
View reviewed changes

Make Later's thunk volatile.

4e81918

This commit also adds a synchronization test, based on Kenji's example. I confirmed that this test fails when the @volatile annotation is removed from thunk.

mikejcurry reviewed May 3, 2016
View reviewed changes

non mentioned this pull request May 30, 2016

Eliminate the use of asInstanceOf in Eval. #924

Closed

non closed this May 30, 2016

non removed the in progress label May 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove internal lazy val from Later. #989

Remove internal lazy val from Later. #989

non commented Apr 20, 2016

ceedubs Apr 20, 2016

non Apr 20, 2016

ceedubs commented Apr 20, 2016

codecov-io commented Apr 20, 2016

non commented Apr 20, 2016

xuwei-k commented Apr 20, 2016

ceedubs commented Apr 20, 2016

non commented Apr 20, 2016

johnynek commented Apr 20, 2016

johnynek commented Apr 20, 2016

johnynek commented Apr 20, 2016

non commented Apr 28, 2016

ceedubs commented May 2, 2016

johnynek commented May 3, 2016

mikejcurry May 3, 2016

johnynek May 3, 2016

mikejcurry May 4, 2016

johnynek May 4, 2016

mikejcurry May 5, 2016

johnynek May 5, 2016

mikejcurry May 5, 2016

non commented May 30, 2016

Remove internal lazy val from Later. #989

Remove internal lazy val from Later. #989

Conversation

non commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs commented Apr 20, 2016

codecov-io commented Apr 20, 2016

Current coverage is 90.73%

non commented Apr 20, 2016

xuwei-k commented Apr 20, 2016

ceedubs commented Apr 20, 2016

non commented Apr 20, 2016

johnynek commented Apr 20, 2016

johnynek commented Apr 20, 2016

johnynek commented Apr 20, 2016

non commented Apr 28, 2016

ceedubs commented May 2, 2016

johnynek commented May 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

non commented May 30, 2016

Current coverage is `90.73%`