-
Notifications
You must be signed in to change notification settings - Fork 3.1k
SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe #4443
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
Conversation
4c1646a
to
034f1b1
Compare
Access was unfortunately relaxed to public because there's no way to emit default access in Scala...
Before:
|
034f1b1
to
33580d4
Compare
33580d4
to
233f7ff
Compare
I cannot explain the required change in bincompat-backward.whitelist.conf -public class scala.concurrent.impl.Promise$DefaultPromise<T extends java.lang.Object> extends scala.concurrent.impl.AbstractPromise implements scala.concurrent.impl.Promise<T>
+public class scala.concurrent.impl.Promise$DefaultPromise<T> extends scala.concurrent.impl.AbstractPromise implements scala.concurrent.impl.Promise<T> Not sure why mima picks up the (irrelevant) change in generic signature (or even why there is a change...) Checking backward binary compatibility for scala-library (against 2.11.0)
Checking forward binary compatibility for scala-library (against 2.11.0)
|
I didn't get to the bottom of the generic signature change yet. Will take another look tomorrow. |
@lrytz: I had the same thought, but I also can't reproduce the signature change.
|
233f7ff
to
a6401fa
Compare
To avoid `sun.misc.Unsafe`, which is not supported on Google App Engine. Deprecate `AbstractPromise` --> extend `j.u.c.atomic.AtomicReference` directly. `AtomicReference.compareAndSet()` should also provide better performance on HotSpot, which compiles it down to the machine's CAS instruction. The binary incompatible change is ok because it's in an internal package. I can't think of any real issue with adding a superclass (which contributes only final methods) to a class in an implementation package (as long as those methods were not introduced in any illicit subclasses of said class). Instead of changing `DefaultPromise`'s super class, let's be more conservative, and do it closest to the source. This is both clearer and more focussed, leaving those subclasses of AbstractPromise we never heard of unaffected. Genesis of the commit: since the work on `Future` performance, `AbstractPromise` is using `Unsafe`, breaking the ability for `Future` to be executed on GAE. At that time, viktorklang suggested to implement a fallback in case `Unsafe` is not available. carey proposed an implementation, and mchv submitted a patch, which was refined by adriaanm.
a6401fa
to
c201eac
Compare
I can no longer reproduce with javap, so can only assume it's a mima bug. I javap'ed --- javap 2015-07-29 22:08:30.000000000 -0700
+++ 2.11.8-c201eac/javap 2015-07-29 22:12:17.000000000 -0700
@@ -1,6 +1,6 @@
-Classfile /private/tmp/scala/concurrent/impl/Promise$DefaultPromise.class
- Last modified Oct 23, 2014; size 14881 bytes
- MD5 checksum d44446cc4a9d37572aa489d9ff98d50e
+Classfile /private/tmp/2.11.8-c201eac/scala/concurrent/impl/Promise$DefaultPromise.class
+ Last modified Jul 29, 2015; size 15119 bytes
+ MD5 checksum 7491676e04834077829f40e3afe48f19
Compiled from "Promise.scala"
public class scala.concurrent.impl.Promise$DefaultPromise<T extends java.lang.Object> extends scala.concurrent.impl.AbstractPromise implements scala.concurrent.impl.Promise<T>
Signature: #3 // <T:Ljava/lang/Object;>Lscala/concurrent/impl/AbstractPromise;Lscala/concurrent/impl/Promise<TT;>;
@@ -11,6 +11,22 @@
public static final #387= #184 of #7; //CompletionLatch=class scala/concurrent/impl/Promise$CompletionLatch of class scala/concurrent/impl/Promise
public static #390= #189 of #389; //InternalCallbackExecutor$=class scala/concurrent/Future$InternalCallbackExecutor$ of class scala/concurrent/Future
Error: unknown attribute
+ ScalaInlineInfo: length = 0xD6 Note there's no diff for |
Any objections to merging, @retronym || @lrytz || @SethTisue? We've been down the "what could possibly be binary incompatible about this" street before, so just quadruple checking :-) |
LGTM. |
LGTM |
SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe
Rebases and slightly reworks #4313.