Content-Length: 382635 | pFad | https://github.com/scala/scala/pull/4443

9D SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe by adriaanm · Pull Request #4443 · scala/scala · GitHub
Skip to content

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

Merged
merged 1 commit into from
Aug 6, 2015

Conversation

adriaanm
Copy link
Contributor

Rebases and slightly reworks #4313.

@scala-jenkins scala-jenkins added this to the 2.11.7 milestone Apr 10, 2015
@adriaanm adriaanm force-pushed the abstractpromise-avoid-unsafe branch from 4c1646a to 034f1b1 Compare April 10, 2015 01:05
@adriaanm
Copy link
Contributor Author

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 adriaanm force-pushed the abstractpromise-avoid-unsafe branch from 034f1b1 to 33580d4 Compare April 14, 2015 05:07
@adriaanm adriaanm force-pushed the abstractpromise-avoid-unsafe branch from 33580d4 to 233f7ff Compare April 21, 2015 17:59
@adriaanm
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@retronym
Copy link
Member

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
Copy link
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
Copy link
Contributor

@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
@adriaanm adriaanm force-pushed the abstractpromise-avoid-unsafe branch from 233f7ff to a6401fa Compare July 29, 2015 23:48
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 adriaanm force-pushed the abstractpromise-avoid-unsafe branch from a6401fa to c201eac Compare July 29, 2015 23:53
@adriaanm
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

LGTM.

@SethTisue
Copy link
Member

LGTM

SethTisue added a commit that referenced this pull request Aug 6, 2015
SI-8362: AbstractPromise extends AtomicReference, avoids sun.misc.Unsafe
@SethTisue SethTisue merged commit 16bc9b7 into scala:2.11.x Aug 6, 2015
@adriaanm adriaanm deleted the abstractpromise-avoid-unsafe branch September 2, 2015 23:19
@retronym retronym added the release-notes worth highlighting in next release notes label Oct 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: https://github.com/scala/scala/pull/4443

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy