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

SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe #4443

Merged
merged 1 commit into from Aug 6, 2015

Conversation

Projects
None yet
7 participants
@adriaanm
Member

adriaanm commented Apr 10, 2015

Rebases and slightly reworks #4313.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 10, 2015

Member

Access was unfortunately relaxed to public because there's no way to emit default access in Scala...

Compiled from "AbstractPromise.scala"
public abstract class scala.concurrent.impl.AbstractPromise extends java.util.concurrent.atomic.AtomicReference<java.lang.Object>
  minor version: 0
  major version: 50
  flags: ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT

Before:

abstract class scala.concurrent.impl.AbstractPromise
  minor version: 0
  major version: 50
  flags: ACC_SUPER, ACC_ABSTRACT
Member

adriaanm commented Apr 10, 2015

Access was unfortunately relaxed to public because there's no way to emit default access in Scala...

Compiled from "AbstractPromise.scala"
public abstract class scala.concurrent.impl.AbstractPromise extends java.util.concurrent.atomic.AtomicReference<java.lang.Object>
  minor version: 0
  major version: 50
  flags: ACC_PUBLIC, ACC_SUPER, ACC_ABSTRACT

Before:

abstract class scala.concurrent.impl.AbstractPromise
  minor version: 0
  major version: 50
  flags: ACC_SUPER, ACC_ABSTRACT
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 21, 2015

Member

I cannot explain the required change in bincompat-backward.whitelist.conf
Bizarrely, scala.concurrent.impl.Promise$DefaultPromise's signature changed?
Looking at it with the REPL's :javap:

-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)

  • the type hierarchy of class scala.concurrent.impl.Promise#DefaultPromise
    has changed in new version. Missing types {java.lang.Object}
 matchName="scala.concurrent.impl.Promise$DefaultPromise"
 problemName=MissingTypesProblem

Checking forward binary compatibility for scala-library (against 2.11.0)
(Note that messages are phrased for backwards compat checks, so flip their relative sense)

  • the type hierarchy of class scala.concurrent.impl.Promise#DefaultPromise
    has changed in new version. Missing types {java.util.concurrent.atomic.AtomicReference}
  • class scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method getState()java.lang.Object in class
    scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method updateState(java.lang.Object,java.lang.Object)Boolean in class
    scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method this()Unit in class scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
Member

adriaanm commented Apr 21, 2015

I cannot explain the required change in bincompat-backward.whitelist.conf
Bizarrely, scala.concurrent.impl.Promise$DefaultPromise's signature changed?
Looking at it with the REPL's :javap:

-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)

  • the type hierarchy of class scala.concurrent.impl.Promise#DefaultPromise
    has changed in new version. Missing types {java.lang.Object}
 matchName="scala.concurrent.impl.Promise$DefaultPromise"
 problemName=MissingTypesProblem

Checking forward binary compatibility for scala-library (against 2.11.0)
(Note that messages are phrased for backwards compat checks, so flip their relative sense)

  • the type hierarchy of class scala.concurrent.impl.Promise#DefaultPromise
    has changed in new version. Missing types {java.util.concurrent.atomic.AtomicReference}
  • class scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method getState()java.lang.Object in class
    scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method updateState(java.lang.Object,java.lang.Object)Boolean in class
    scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
  • method this()Unit in class scala.concurrent.impl.AbstractPromise was public;
    is inaccessible in new version
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Apr 21, 2015

Member

I hesitate to merge this after triple-checking. @retronym, @lrytz, any thoughts on the unexpected signature change for scala.concurrent.impl.Promise$DefaultPromise?

Member

adriaanm commented Apr 21, 2015

I hesitate to merge this after triple-checking. @retronym, @lrytz, any thoughts on the unexpected signature change for scala.concurrent.impl.Promise$DefaultPromise?

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Apr 22, 2015

Member

I didn't get to the bottom of the generic signature change yet. Will take another look tomorrow.

Member

retronym commented Apr 22, 2015

I didn't get to the bottom of the generic signature change yet. Will take another look tomorrow.

@retronym retronym self-assigned this Apr 22, 2015

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Apr 22, 2015

Member

I tried to reproduce the signature change but couldn't. Somehow it reminds me of #4403, and also #4080.

Member

lrytz commented Apr 22, 2015

I tried to reproduce the signature change but couldn't. Somehow it reminds me of #4403, and also #4080.

@gourlaysama

This comment has been minimized.

Show comment
Hide comment
@gourlaysama

gourlaysama Apr 22, 2015

Member

@lrytz: I had the same thought, but I also can't reproduce the signature change.
I see the signature below both before and after this change for Promise$DefaultPromise (and in 2.11.0 too):

<T:Ljava/lang/Object;>Lscala/concurrent/impl/AbstractPromise;Lscala/concurrent/impl/Promise<TT;>;
Member

gourlaysama commented Apr 22, 2015

@lrytz: I had the same thought, but I also can't reproduce the signature change.
I see the signature below both before and after this change for Promise$DefaultPromise (and in 2.11.0 too):

<T:Ljava/lang/Object;>Lscala/concurrent/impl/AbstractPromise;Lscala/concurrent/impl/Promise<TT;>;

@adriaanm adriaanm assigned adriaanm and unassigned retronym May 22, 2015

@SethTisue SethTisue modified the milestones: 2.11.8, 2.11.7 Jun 17, 2015

SI-8362: AbstractPromise extends AtomicReference
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.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 30, 2015

Member

I can no longer reproduce with javap, so can only assume it's a mima bug. I javap'ed scala.concurrent.impl.Promise$DefaultPromise from 2.11.4 (don't ask) and this commit:

--- 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 public class scala.concurrent.impl.Promise$DefaultPromise<T extends java.lang.Object> extends scala.concurrent.impl.AbstractPromise implements scala.concurrent.impl.Promise<T>

Member

adriaanm commented Jul 30, 2015

I can no longer reproduce with javap, so can only assume it's a mima bug. I javap'ed scala.concurrent.impl.Promise$DefaultPromise from 2.11.4 (don't ask) and this commit:

--- 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 public class scala.concurrent.impl.Promise$DefaultPromise<T extends java.lang.Object> extends scala.concurrent.impl.AbstractPromise implements scala.concurrent.impl.Promise<T>

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 30, 2015

Member

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 :-)

Member

adriaanm commented Jul 30, 2015

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 :-)

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 30, 2015

Member

LGTM.

Member

retronym commented Jul 30, 2015

LGTM.

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Jul 30, 2015

Member

LGTM

Member

SethTisue commented Jul 30, 2015

LGTM

SethTisue added a commit that referenced this pull request Aug 6, 2015

Merge pull request #4443 from adriaanm/abstractpromise-avoid-unsafe
SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe

@SethTisue SethTisue merged commit 16bc9b7 into scala:2.11.x Aug 6, 2015

5 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
integrate-ide [1346] SUCCESS. Took 89 min.
Details
validate-main [1651] SUCCESS. Took 100 min.
Details
validate-publish-core [1591] SUCCESS. Took 10 min.
Details
validate-test [1109] SUCCESS. Took 49 min.
Details

@adriaanm adriaanm deleted the adriaanm:abstractpromise-avoid-unsafe branch Sep 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment