Feature/future compat #454
Conversation
Fixed a bug in Future.zip.
Ported most of the future tests.
…anBuildFrom.
Removed the implicit modifier on the OnceCanBuildFrom, as we don't
support implicit classes with zero arguments.
Added an implicit OnceCanBuildFrom method.
The idea behind OnceCanBuildFrom is for it to be used by methods which
construct collections, but are defined outside of collection classes.
OnceCanBuildFrom so far worked only for objects of type TraversableOnce:
shuffle(List(1, 2, 3).iterator: TraversableOnce[Int])
but this used to result in an implicit resolution error:
shuffle(List(1, 2, 3).iterator)
because after the type parameter M for `shuffle` was inferred to Iterator, no implicit
of type CanBuildFrom[Iterator[_], A, Iterator[A]] could be found.
Introduced another CanBuildFrom to the Iterator companion object.
Modified Future tests appropriately.
The builder is now instantiated as an iterator builder only if a generic builder cannot be found on the collection that requested the builder. Reason - we want this behaviour: scala> scala.util.Random.shuffle(List(1, 2, 3): collection.TraversableOnce[Int]) res0: scala.collection.TraversableOnce[Int] = List(3, 1, 2) instead of this one: scala> scala.util.Random.shuffle(List(1, 2, 3): collection.TraversableOnce[Int]) res0: scala.collection.TraversableOnce[Int] = non-empty iterator which may lead to nasty surprises. Alternately, to avoid pattern-matching in OnceCanBuildFrom.apply, we could mix in GenericTraversableTemplate-related functionaly into TraversableOnce, but this may become too complicated.
|
|
||
| def defaultReporter: Throwable => Unit = { | ||
| // `Error`s are currently wrapped by `resolver`. | ||
| // Also, re-throwing `Error`s here causes an exception handling test to fail. |
There was a problem hiding this comment.
I think I didn't write that comment, just copied it.
Aleksandar Prokopec
LAMP, IC, EPFL
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
viktorklang reply@reply.github.com wrote:
/** Creates an
ExecutionContextfrom the givenExecutor.
*/
- def fromExecutor(e: Executor): ExecutionContext with Executor = new impl.ExecutionContextImpl(e)
- def fromExecutor(e: Executor, reporter: Throwable => Unit = defaultReporter): ExecutionContext with Executor = new impl.ExecutionContextImpl(e, reporter)
- def defaultReporter: Throwable => Unit = {
- //
Errors are currently wrapped byresolver.- // Also, re-throwing
Errors here causes an exception handling test to fail.
Which test?
Reply to this email directly or view it on GitHub:
https://github.com/scala/scala/pull/454/files#r758713
There was a problem hiding this comment.
Can the offender explain him/herself? ;-)
|
@axel22, @viktorklang I'm not sure this is the only issue that keeps us from running on google app engine, but the change from |
|
We use Unsafe for performance and compatibility reasons (performance in the sense that the FieldUpdaters are considerably slower due to their design, this has been frequently discussed on concurrency-interest) and compatibility in the sense that our embedded version of ForkJoinPool is written using Unsafe and switching to FieldUpdaters would not only make updates to the FJP code riskier (the FJP code is extremely hard to maintain, have a look) as well as make it slower, which is detrimental for an execution engine). If someone has a potential solution with a back-off strategy that tries to use Unsafe but falls back to FieldUpdaters if there is any issues, that doesn't carry any runtime performance penalty, I'm all ears! Also, since Akka also heavily uses Unsafe it wouldn't work on GAE either. In any case, have we stated GAE compliance? (In which case we should run automated tests on GAE) |
|
We don't promise GAE support, but it would be nice if we could if the cost is low. |
|
I assume the JIT will quickly get rid of the if on a final static boolean(?). |
|
Could work, but it's a lot of ceremony and is error prone (X fields and Y different uses (CAS, getAndSet etc)). Can we work out a solution where we can get to: private final static IntUpdater _ref = Updater.create(AbstractPromise.class, "_ref"); And make sure that things will get inlined and intrinsified properly, then we're all good. |
|
Would make a ton of sense to create something like this, private to the Scala StdLib. something like: |
|
I agree that would be ideal & really cool. I guess I got a little carried away, let's schedule considering a minimal fix for 2.11.1, as it seems it would be binary compatible anyway. |
|
I'm kind of intrigued by this, so I read the thread you alluded to and tried to encapsulate the fallback to the updater in another class. It seems that the whole point, though, is better performance, so we don't want to add any indirection. Thus, it seems unwise to encapsulate the CAS behaviour in a new By the way, Alternative updater below because I already wrote all this stuff down anyway and I'd feel less guilty about spending time on this if at least there's a record of some sort :-) object FastAtomicUpdater {
def apply[T, V](tclass: Class[T], vclass: Class[V], fieldName: String): AtomicReferenceFieldUpdater[T, V] =
try new AtomicUpdaterUnsafe(Unsafe.instance.objectFieldOffset(tclass.getDeclaredField(fieldName)))
catch { case _ : Throwable => // ok ok, should be NonFatal
// fallback to AtomicReferenceFieldUpdater if Unsafe is not available, e.g. on GAE (SI)
AtomicReferenceFieldUpdater.newUpdater(tclass, vclass, fieldName)
}
}
final class AtomicUpdaterUnsafe[T, V](fieldOffset: Long) extends AtomicReferenceFieldUpdater[T, V] {
def compareAndSet(target: AnyRef, oldState: AnyRef, newState: AnyRef) =
Unsafe.instance.compareAndSwapObject(target, fieldOffset, oldState, newState)
// ...
} |
|
(I reported the issue on the Scala Jira.) I have Scala code running perfectly well on Google App Engine, without any threading for now. Even single-threaded we're not exceeding the App Engine request deadlines at this point, and if I did want to use multiple threads I could use the
I expect this could be written in Scala, but I didn't want to complicate things. |
|
Whatever we do, we need to make sure we use Unsafe if it's available; and that the overhead will be that of what it currently is, i.e. it gets inlined and intrinsified properly) Thanks for being on top of this Adriaan |
|
@carey, thanks for investigating! Any chance you could write this up as a PR or detail your investigation? How do you verify the cmpxchg gets emitted & inlined? (I'm more of a front-end guy.) I think it would be nice (literally, "nice", since we don't really officially promise this) not to regress on our GAE support. |
|
Hotspot has an option to disassemble and dump the assembly code that it generates; in this case I used leaving out how the I can write up some more details of my testing later. |
|
@adriaanm front-end guy -> "front man." Also, "nice" literally: |
|
Interesting evolution for such a small word. I guess I'd call myself a nice (renaissance) man then. Also, innocent in this particular pr. |
|
Agree with everything said - if we could have our own |
|
Sounds great, I'm happy to review any PR that tries to address this. |
|
My testing was slowed down by finding that the Java 6 Hotspot compiler is quite a bit worse than Java 7, and can't see past abstract classes or checking against null. The best option is maybe the simplest; if |
|
Manually fiddling with offsets means duplicating this code whenever/wherever (we're meant to be together — Shakira) Unsafe is used. I'm much more in favor of subclassing the Updaters if we can get it to inline and intrinsify properly. |
|
The Java 6 Client VM doesn't inline alternate subclasses, as at https://github.com/carey/hotspot-tests/blob/444cd0c64d030cbfa8b78fdc6a5b5f967e99e53e/hotspot-tests/src/nz/geek/carey/unsafe/abstractclasses/BaseClass.java. Only 32-bit Java on Windows uses the client VM, where threading performance of Scala code is probably not a major concern, especially when it only seems to be Java 6. See https://gist.github.com/carey/9444544 for the compiler output, which I saved this time. |
|
I agree with the comment on client vm for Java 6. How does it behave with Tiered Compilation? |
|
I haven't been able to see any difference in my tests between the client VM and tiered compilation in Java 6 and Java 7, and Java 8 says: |
|
You have to use -server to use TieredCompilation. There is a tiered policy that determines which compiler gets used when, see http://hg.openjdk.java.net/jdk7/jdk7/hotspot/file/5d8f5a6dced7/src/share/vm/runtime/advancedThresholdPolicy.cpp for an example. |
|
I am experimenting with this for SIP-23: https://github.com/viktorklang/scala/pull/6 Sadly, JSR166 relies on things that are not exposed on AtomicXFieldUpdaters (like relaxed writes), which means that there be uglies. Also, I see a huge risk in modifying JSR166 classes (as well as the investment into testing and verifying UnsafeAtomicXUpdaters). |
|
Thanks for the update, @viktorklang. |
* * Add tests of `force` for LazyList and Stream. * Test string representations of LazyList and Stream after `force` call. * Add tests of `sameElements` for LazyList and Stream. * Change `force` result type from `Option[(head, tail)]` to `CC[A]`. * Force `force` to evaluate whole LazyList and Stream. * Update methods depended on `force`: `unapply` and `sameElements`. * Reimplement `sameElements`. ** Remove Tuple allocation. ** Check reference equality on every step. * * Code style. * * Remove redundant override of `sameElements` method. * * Set more strict type of `force` method. * * Stack safe `force` with cyclic references detection. * * Update LazyListForceBenchmark * * Fix method name. * Test `sameElements` with cyclic collections. * Reduce allocation in `force` method.
Mostly tests for the scala.concurrent package, and a couple of bugfixes.