[minor] Add pool.onQuiescent(cell) #93
Conversation
JanKoelzer
changed the title
| success = poolState.compareAndSet(state, newState) | ||
| } else if (state.submittedTasks == 1) { | ||
| handlersToRun = Some(state.handlers) | ||
| handlersToRun = Some(state.quiescenceHandlers) |
There was a problem hiding this comment.
This appears to be missing state.quiescenceCellHandlers. Otherwise, quiescence cell handlers are only run upon registration if the pool is already quiescent at that point.
Also, it would be good to add a test which would catch this issue (i.e., register quiescence cell handler when the pool is definitely not quiescent, and verify the handler runs eventually).
core/src/test/scala/pool.scala
Outdated
| @@ -2,11 +2,15 @@ package cell | |||
|
|
|||
| import java.util.concurrent.ConcurrentHashMap | |||
|
|
|||
| import java.util.concurrent.CountDownLatch | |||
There was a problem hiding this comment.
Combine with previous import clause.
core/src/test/scala/pool.scala
Outdated
| val p1 = Promise[Boolean]() | ||
| val p2 = Promise[Boolean]() | ||
| pool.execute { () => { p1.success(true) }: Unit } | ||
| pool.onQuiescent { () => p2.success(true) } |
There was a problem hiding this comment.
The type ascription in the call to execute is not so nice. Can we get rid of it?
core/src/test/scala/pool.scala
Outdated
| val p1 = Promise[Boolean]() | ||
| val p2 = Promise[Boolean]() | ||
| pool.execute { () => { p1.success(true) }: Unit } | ||
| pool.onQuiescent { () => p2.success(true) } |
| val quiescenceHandlers: List[() => Unit] = List(), | ||
| val quiescenceCellHandlers: List[NextCallbackRunnable[_, _]] = List(), | ||
| val submittedTasks: Int = 0) { | ||
| def isQuiescent: Boolean = |
There was a problem hiding this comment.
It would be better not to remove the pair of parens (), because isQuiescent is not pure (its result depends on the state of the pool). The Scala convention is to always have a pair of parens for impure methods.
| def isQuiescent(): Boolean = | ||
| private class PoolState( | ||
| val quiescenceHandlers: List[() => Unit] = List(), | ||
| val quiescenceCellHandlers: List[NextCallbackRunnable[_, _]] = List(), |
There was a problem hiding this comment.
Why do we store NextCallbackRunnables, and not just closures like with quiescenceHandlers? One thing I'm a bit skeptical about is that NextCallbackRunnable[_, _] is a wildcard type, adding complexity which I'm not sure is warranted.
| @@ -12,11 +12,18 @@ import lattice.{ DefaultKey, Key, Updater } | |||
| import org.opalj.graphs._ | |||
|
|
|||
| import scala.collection.immutable.Queue | |||
| import scala.util.{ Success, Try } | |||
JanKoelzer
force-pushed
the
onQcell
branch
2 times, most recently
from
4f44d4a to
367f4bb
Compare
|
All comments are displayed as outdated, which is not correct (prob. because of the new package/file structure).
--You are right. I am going to address this.-- This issue should be fixed now, two tests have been added.
Along with every callback, we need to store the cell whose value is passed to the callback as soon as quiescence is reached. That's quite exactly, what NextCallbackRunnable is used for in
The imports you commented on are used. Am I missing unused once? |
Fixes #89 (but see discussion at #89 )