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.
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
STM supports scala.js #1338
STM supports scala.js #1338
Conversation
…er executing some op with lock (zio#803)
Great work! Could you move the STM tests from |
I moved the STM tests from [error] Referring to non-existent method java.util.concurrent.CountDownLatch.<init>(scala.Int)
[error] called from zio.stm.STMSpec.$$anonfun$e28$1()zio.ZIO
[error] called from zio.stm.STMSpec.e28()org.specs2.matcher.MatchResult
[error] called from zio.stm.STMSpec.$$anonfun$is$27()org.specs2.matcher.MatchResult
[error] called from zio.stm.STMSpec.$$anonfun$is$1()org.specs2.specification.core.Fragments
[error] called from zio.stm.STMSpec.is()org.specs2.specification.core.SpecStructure
[error] called from org.specs2.specification.core.SpecificationStructure.$$anonfun$structure$1(org.specs2.specification.core.Env)org.specs2.specification.core.SpecStructure
[error] called from zio.FunctionIOSpec.$$anonfun$structure$1(org.specs2.specification.core.Env)org.specs2.specification.core.SpecStructure
[error] called from org.specs2.specification.core.SpecificationStructure.structure()scala.Function1
[error] called from org.specs2.Specification.org$specs2$specification$core$ImmutableSpecificationStructure$$super$structure()scala.Function1
[error] called from org.specs2.specification.core.ImmutableSpecificationStructure.$$anonfun$structure$1(org.specs2.specification.core.Env)org.specs2.specification.core.SpecStructure
[error] called from zio.FunctionIOSpec.$$anonfun$structure$1(org.specs2.specification.core.Env)org.specs2.specification.core.SpecStructure
[error] called from org.specs2.specification.core.ImmutableSpecificationStructure.structure()scala.Function1
[error] called from org.specs2.Specification.structure()scala.Function1
[error] called from org.specs2.runner.SbtTask.$$anonfun$createSpecStructure$2(org.specs2.specification.core.Env,org.specs2.specification.core.SpecificationStructure)scala.Option
[error] called from org.specs2.runner.SbtTask.createSpecStructure(sbt.testing.TaskDef,java.lang.ClassLoader,org.specs2.specification.core.Env)org.specs2.control.eff.Eff
[error] called from org.specs2.runner.SbtTask.executeFuture(sbt.testing.EventHandler,[sbt.testing.Logger)scala.concurrent.Future
[error] called from org.specs2.runner.SbtTask.execute(sbt.testing.EventHandler,[sbt.testing.Logger,scala.Function1)scala.Unit
[error] called from org.scalajs.testinterface.internal.Bridge$.$$anonfun$executeFun$1(sbt.testing.Runner,scala.Int,org.scalajs.testcommon.ExecuteRequest)scala.concurrent.Future
[error] called from org.scalajs.testinterface.internal.Bridge$.executeFun(scala.Int,sbt.testing.Runner)scala.Function1
[error] called from org.scalajs.testinterface.internal.Bridge$.$$anonfun$createRunnerFun$1(scala.Boolean,org.scalajs.testcommon.RunnerArgs)scala.Unit
[error] called from org.scalajs.testinterface.internal.Bridge$.createRunnerFun(scala.Boolean)scala.Function1
[error] called from org.scalajs.testinterface.internal.Bridge$.start()scala.Unit
[error] called from org.scalajs.testinterface.internal.Bridge$.__exportedInits
[error] exported to JavaScript with @JSExport
[error] involving instantiated classes:
[error] zio.stm.STMSpec
[error] zio.FunctionIOSpec
[error] zio.internal.OneShotSpec
[error] zio.PromiseSpec
[error] zio.FiberLocalSpec
[error] zio.duration.DurationSyntaxSpec
[error] zio.RetrySpec
[error] zio.RepeatSpec
[error] zio.RefMSpec
[error] zio.internal.ExecutorSpec
[error] zio.ParallelErrorsSpec
[error] zio.internal.MutableConcurrentQueueSpec
[error] zio.FiberRefSpec
[error] zio.RefSpec
[error] zio.duration.DurationSpec
[error] zio.FiberSpec
[error] zio.internal.StackBoolSpec
[error] zio.QueueSpec
[error] zio.ExitSpec
[error] org.specs2.runner.SbtTask
[error] org.scalajs.testinterface.internal.Bridge$
.
.
. It seems that |
I got away with [error] java.util.concurrent.ExecutionException: Boxed Error I'm trying to fix this. |
@heraklos we could use a For example: def e21 =
unsafeRun {
for {
latch <- Promise.make[Nothing, Unit]
done <- Promise.make[Nothing, Unit]
tvar1 <- TRef.makeCommit(0)
tvar2 <- TRef.makeCommit("Failed!")
fiber <- (STM.atomically {
for {
v1 <- tvar1.get
_ <- STM.succeedLazy(unsafeRun(latch.succeed(())))
_ <- STM.check(v1 > 42)
_ <- tvar2.set("Succeeded!")
v2 <- tvar2.get
} yield v2
} <* done.succeed(())).fork
_ <- latch.await
old <- tvar2.get.commit
_ <- tvar1.set(43).commit
_ <- done.await
newV <- tvar2.get.commit
join <- fiber.join
} yield (old must_=== "Failed!") and (newV must_=== join)
} |
Oops, you found it already. Are you getting those errors only with ScalaJS? |
@ghostdogpr And yes to your question. The error pops out only when executing ScalaJS test.
in errors shown in the stack trace, and this message seems to come from |
ea79bcb
to
76dee8e
Compare
Maybe those 3 tests that target concurrent behavior should stay in JVM since they don't make too much sense in JS? @jdegoes what do you think? |
The whole STM test suite should run on Scala.js without issue. But Specs 2 supports returning Futures, so we need to Does that make sense? |
Thank you for your guide! |
76dee8e
to
639fdf8
Compare
639fdf8
to
e6eeca5
Compare
@ghostdogpr
I think my changes does not affect |
Yeah it’s probably the same issue as #1216. I restarted it. |
Thanks a lot 😆 But another flaky test popped up... I'm sure this is the same issue as #1023
Would you run tests again? |
It’s green now :D |
LGTM, @jdegoes you wanna have a final look? |
|
||
try if (isValid(journal)) commitJournal(journal) else loop = true | ||
finally globalLock.release() | ||
globalLock.synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought .synchronized
threw exceptions on Scala.js. No???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. java built-in synchronized
throws exception on Scala.js.
So, I implemented and used synchronized
method on LockedRef
class which wraps the procedure in {}
code with locking/unlocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heraklos How about this:
// JS
object Sync {
def apply[A](anyRef: AnyRef)(f: => A): A = f
// JVM
object Sync {
def apply[A](anyRef: AnyRef)(f: => A): A = anyRef.synchronized { f }
}
Then you can replace x.synchronized { f }
with Sync(x) { f }
Sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdegoes
That sounds perfect to me!
And with Sync
, STMSpec
passed for both jvm
/js
!
The only obstacle here is the following error
parameter value anyRef in method apply is never used
for Sync
of js
.
// JS
object Sync {
def apply[A](anyRef: AnyRef)(f: => A): A = f
}
I'm gonna think of some workaround.
|
||
def lock(): Unit = { | ||
var loop = true | ||
while (loop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This spin loop is not efficient. You should just use an ordinary Java Semaphore
or Lock
. An alternative would be to implement a lock atop synchronized
, which would be more efficient than anything else. You have to know how to use waitAll
/ notifyAll
and deal with spurious wakeups, etc. So the simple solution is to use Semaphore
here the more complex one is to build something low-level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your advice on locking!
Unfortunately, java.util.concurrent.Semaphore
is not supported by scala.js...
Maybe I should implement lock with ReentrantLock
here while learning waitAll
/ notifyAll
and other concurrent matter to be cared for, since I'm not familiar with those concepts. (java's built-in synchronized
is not supported as you taught us in #803)
But on reading your overall review on this PR, this class is not needed in the first place...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javascript will not need a semaphore. I think we can get by with purely synchronized
(and Sync
class above), without any waiting / awaiting.
Can you give it a try and let me know? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're totally correct.
The mechanism of Javascript runtime slipped my mind.
|
||
import java.util.concurrent.atomic.AtomicBoolean | ||
|
||
class LockedRef[A](var ref: A) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this raw var
is stored here, it needs to be marked as @volatile.
@@ -561,7 +570,7 @@ object STM { | |||
|
|||
private[this] val txnCounter: AtomicLong = new AtomicLong() | |||
|
|||
final val globalLock = new java.util.concurrent.Semaphore(1) | |||
final val globalLock = new zio.internal.LockedRef(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not even using the Int
passed into LockedRef
, which means globalLock
is unnecessary. You may as well just synchronize on STM
object, e.g. STM.synchronized { ... }
or maybe make a private object, e.g. final val globalLock = new AnyRef { }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! This LockedRef
instance does not use the passed Int
at all.
but my understanding is that we can't use java's built-in synchronized
in scala.js.
So, I have to come up with some data structure similar to LockedRef
which does not take an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work on this! Supporting Scala.js is very critical to 1.0 and this moves us a long way there.
Upon review of this pull request, I don't think we need LockedRef.scala
. Indeed, no methods are used, it's just used as a container.
Instead, I think we can get by simple with AtomicBoolean
+ synchronized
for done
, and a val globalLock: AnyRef = new AnyRef { }
for the global lock (using synchronized
like you are currently doing).
This will greatly reduce code and improve performance and should be a quick change to make.
Let me know if you have any questions!
@jdegoes But here is the problem. I actually used So I'm afraid that we can get by simple with Instead, what about if separating |
package zio.internal | ||
|
||
object Sync { | ||
def apply[A](anyRef: AnyRef)(f: => A): A = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For STM.scala
to be shared, this method should take the same arguments as that of Sync
in jvm
does.
But the compiler does not allow us to take anyRef
and does nothing with it.
So I avoided the error just by asserting the existence of anyRef
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do, inside the body:
val _ = anyRef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! Thank you for your patience on this large PR!
@heraklos Glad to see you went through this 👏👏👏 One less blocker for ZIO 1.0! |
@jdegoes @ghostdogpr |
@heraklos Congratulations on your first merge to core... first of many, I hope! 😉 |
Issue : #803
now scala.js code with STM like the following works
Changes in code
Implement
LockedRef
classsychronized
andjava.util.concurrent.Semaphore
for locking, which are not supported in scala.jsReplace
forEach
method calls with iterator-based implementationHashMap
as of nowReplace
synchronized
andjava.util.concurrent.Semaphore
withLockedRef
Changes in performance
sbt:zio> benchmarks/jmh:run .*SemaphoreBenchmark.*
task locallymaster branch
feature branch
the result is that average ops/s on
zio.stm.SemaphoreBenchmark.semaphoreCatsContention
andzio.stm.SemaphoreBenchmark.semaphoreContention
got worse for about 5 ops/s as expected, and average ops/s onzio.stm.SemaphoreBenchmark.tsemaphoreContention
unexpectedly got better for about 15 ops/s