8000 SI-8362: Support for Future without sun.misc.Unsafe. by mchv · Pull Request #4313 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

SI-8362: Support for Future without sun.misc.Unsafe. #4313

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

SI-8362: Support for Future without sun.misc.Unsafe. #4313

wants to merge 2 commits into from

Conversation

mchv
Copy link
Contributor
@mchv mchv commented Feb 10, 2015

Since the work on Future performances, 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, but as far as I know he never created a pull request for it, so there it is.

Please comment if some improvement or changes are needed, so I could try to improve.

See SI-8362 for more details.

@mchv
SI-8362: Support for Future without sun.misc.Unsafe.
f4424c8 
Instead of raising ExceptionInInitializerError, set the field offset to an invalid value,
then check for that value when updating the state, falling back to the official but
slower AtomicReferenceFieldUpdater. Hotspot is smart enough to inline only the
compareAndSwapObject intrinsic in the caller, and ignore the fallback code.
@scala-jenkins scala-jenkins added this to the 2.11.6 milestone Feb 10, 2015
@adriaanm
Copy link
Contributor

/rebuild -- having issues with jenkins spuriously disconnecting from workers

@viktorklang
Copy link
Contributor

I think I prefer the 2.12.x solution: https://github.com/scala/scala/blob/2.12.x/src/library/scala/concurrent/impl/Promise.scala#L181

@carey
Copy link
carey commented Feb 11, 2015

I had a lot of trouble getting the Scala build tests to run, and getting Hotspot to tell me if it wasn't getting any slower, and eventually I gave up.

For the 2.12.x solution, that's extending AtomicReference instead of using AtomicReferenceFieldUpdater? That's much more elegant.

Until 2.12.x is released and Google App Engine supports Java 8, at least this change here would make Scala work on GAE without changing the superclass of AbstractPromise.

@He-Pin
Copy link
Contributor
He-Pin commented Feb 11, 2015

in fact AtomicReferenceFieldUpdater save bytes.
http://normanmaurer.me/blog/2013/10/28/Lesser-known-concurrent-classes-Part-1/

@carey
Copy link
carey commented Feb 11, 2015

Not the way Scala 2.12.x does it. 2.11's AbstractPromise contains a single field _ref, while 2.12 extends AtomicReference which contains a single field value. However, AtomicReferenceFieldUpdater.compareAndSet() isn't necessarily even inlined by Hotspot, while AtomicReference.compareAndSet() should be compiled down to the machine's CAS instruction.

I'm quite impressed by how Scala 2.12.x does it.

For this pull request, as far as I could tell the Hotspot compiler would always notice that _ref was always -1, and generate the same code as before the patch. On GAE it will be a bit slower, but you wouldn't use that for raw performance.

@viktorklang
Copy link
Contributor

@carey Thanks for the kind words regarding the 2.12.x implementation. What about the following proposal: Remove the Unsafe-usage from AbstractPromise in 2.11.x and make AbstractPromise extend AtomicReference?

@He-Pin
Copy link
Contributor
He-Pin commented Feb 12, 2015

if there is large instance of Promise,using the AtomicReferenceFieldUpdater will reduce the memory footprint.but one drawback is you have to write java code.

@viktorklang
Copy link
Contributor

@hepin1989 Yes, compared to composing an AtomicX, but not extending one.

@carey
Copy link
carey commented Feb 12, 2015

The project manager in me isn't happy with a change like that in a minor release; on the other hand, I've just tested a simple version of it on App Engine and it works fine:

package scala.concurrent.impl;

class AbstractPromise extends java.util.concurrent.atomic.AtomicReference<Object> {
    protected final boolean updateState(Object oldState, Object newState) {
        return compareAndSet(oldState, newState);
    }

    protected final Object getState() {
        return get();
    }
}

@viktorklang
Copy link
Contributor

@carey Given that it is a package-protected class in an impl package, it should be safe. @adriaanm wdyt?

@adriaanm
Copy link
Contributor

So, what exactly would the 2.12 solution entail? Adding a new package-protected class to an impl package? (I tried to figure it out based on the commits/linked diff, but not sure.)

@viktorklang
Copy link
Contributor

@adriaanm Like: #4313 (comment)

@mchv
Copy link
Contributor Author
mchv commented Feb 14, 2015

I was suspicious changing implementation to match 2.12.x will break binary compatibility, and effectively it does:

Checking backward binary compatibility for scala-library (against 2.11.0)
     [mima] Found 1 binary incompatibiities (3 were filtered out)
     [mima] =====================================================
     [mima]  * the type hierarchy of class scala.concurrent.impl.Promise#DefaultPromise
     [mima]    has changed in new version. Missing types {java.lang.Object}
     [mima] Generated filter config definition
     [mima] ==================================
     [mima] 
     [mima]     filter {
     [mima]         problems=[
     [mima]             {
     [mima]                 matchName="scala.concurrent.impl.Promise$DefaultPromise"
     [mima]                 problemName=MissingTypesProblem
     [mima]             }
     [mima]         ]
     [mima]     }
     [mima]

I have used following implementation

package scala.concurrent.impl;


import java.util.concurrent.atomic.AtomicReference;



abstract class AbstractPromise extends AtomicReference<Object> {

    protected final boolean updateState(Object oldState, Object newState) {
        return compareAndSet(oldState, newState);
    }

    protected final Object getState() {
        return get();
    }
}

@viktorklang
Copy link
Contributor

@mchv

DefaultPromise is in (https://github.com/scala/scala/blob/2.11.x/src/library/scala/concurrent/impl/Promise.scala#L153) which is private[concurrent] so I don't see that it would cause any issues, but @adriaanm would know more.

@mchv
Copy link
Contributor Author
mchv commented Feb 16, 2015

The jenkins failing build log of AbstractPromise extending AtomicReference (see #4313 (comment))

8000
@viktorklang
Copy link
Contributor

@mchv Yep, so it needs a mima exception added.

@mchv
Copy link
Contributor Author
mchv commented Feb 16, 2015

@viktorklang It looks to me that ignoring private classes is currently unsupported by mima, do you know if there is a way to specify explicitely a qualified classname to ignore?

@viktorklang
Copy link
Contributor

@mchv I'm unsure, perhaps @dotta could elucidate us.

@mchv
Copy link
Contributor Author
mchv commented Feb 16, 2015

@viktorklang @dotta nevermind I just found documentation about it and associated filters file in codebase. I will update the pull request

@mchv
SI-8362: Support for Future without sun.misc.Unsafe.
30a6b69 
Instead of using AtomicReferenceFieldUpdater extends directly AtomicReference as AtomicReferenceFieldUpdater.compareAndSet() isn't necessarily even inlined by Hotspot, while AtomicReference.compareAndSet() should be compiled down to the machine's CAS instruction.
The implementation is similar to the 2.12.x branch’s one.

Add an exclusion rule for mima to not check backward and forward compatibility as the class is package protected and in an imp package.
@mchv
Copy link
Contributor Author
mchv commented Feb 17, 2015

@dotta may you review my changes related to mima?

@dotta
Copy link
Contributor
dotta commented Feb 17, 2015

@mchv The filter rules seems correct (and the build bot is happy).

Maybe one day someone will work on lightbend-labs/mima#34.

@adriaanm
Copy link
Contributor

In general, 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).

I think this 8000 actual plays in favor of @viktorklang's solution in 2.12. If we're going to change superclasses, why not do it closest to the source? This is both clearer and more focussed, leaving those subclasses of AbstractPromise we never heard of unaffected. We could also deprecate AbstractPromise -- even in 2.11.6, since it's an internal class.

EDIT: forgot that java classes are package protected by default (in casu, AbstractPromise)

Since binary compatibility mistakes can tank whole releases, I'd like a second opinion from the others in @scala/team-core-scala

@adriaanm adriaanm modified the milestones: 2.11.6, 2.11.7 Feb 19, 2015
@adriaanm adriaanm removed this from the 2.11.6 milestone Feb 19, 2015
@adriaanm
Copy link
Contributor

Since the 2.11.6 deadline is extremely nigh, I've tentatively postponed this PR (along with other recent ones) to 2.11.7. I'll make a pass through reviewed 2.11.7 PRs on Friday and move them back to 2.11.6 where feasible.

In the mean time, feel free to let me know if you'd like some help getting this reviewed by then.

@mchv
Copy link
Contributor Author
mchv commented Mar 24, 2015

@adriaanm @scala/team-core-scala Any update on planning to merge this for 2.11.7 ?

@gkossakowski
Copy link
Contributor

FYI: I'll give binary compatibility thought today.

@lrytz
Copy link
Member
lrytz commented Mar 25, 2015

A burnt child dreads the fire. Still, to me it looks good in terms of binary compatibility.

@gkossakowski
Copy link
Contributor

I also we are fine here when it comes to binary compatibility.

@adriaanm adriaanm self-assigned this Mar 30, 2015
@adriaanm
Copy link
Contributor

I'll adopt this PR. Going to have a look at some jars in the wild to see if there are any dependencies we may have missed.

@Ichoran
Copy link
Contributor
Ichoran commented Apr 1, 2015

It doesn't take much code to generate a problem:

package scala.concurrent.impl
class AbPro extends AbstractPromise { def u = AbstractPromise.updater }

But I guess people normally don't do that (or we don't support them doing that; that's not what we mean by "binary compatibility"), so otherwise it should be good. The optimizer doesn't seem to slurp anything untoward into the bytecode for concurrent.Promise.

@adriaanm
Copy link
Contributor
adriaanm commented Apr 1, 2015

Indeed -- i was going to just grep for any mention of scala.concurrent.impl.AbstractPromise. If you still have those jars expanded, maybe that would be convenient for you to pick up?

@Ichoran
Copy link
Contributor
Ichoran commented Apr 1, 2015

@adriaanm - I'm not sure where they went, actually. I think I might have copied them from a USB drive to /tmp, then lost the USB drive. I can do it anyway, though, if you'd like. (Might need to tell me the best incantation to grab the right jars from Maven.)

@adriaanm
Copy link
Contributor
adriaanm commented Apr 1, 2015

Ok, no prob -- I'll look into it then.

@adriaanm adriaanm mentioned this pull request Apr 10, 2015
Merged
@adriaanm
Copy link
Contributor

Closing in favor of the rebased & slightly reworked #4443.

@adriaanm adriaanm closed this Apr 10, 2015
@adriaanm adriaanm added the welcome hello new contributor! label May 26, 2015
@scabug scabug mentioned this pull request Apr 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@adriaanm adriaanm

Labels
welcome hello new contributor!
Projects
None yet
Milestone
2.11.7
Development

Successfully merging this pull request may close these issues.
10 participants
@mchv @adriaanm @viktorklang @carey @He-Pin @dotta @gkossakowski @lrytz @Ichoran @scala-jenkins
0