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

Remove our fork of forkjoin. Java 8 bundles it. #4629

Merged
merged 2 commits into from Jul 20, 2015

Conversation

Projects
None yet
7 participants
@adriaanm
Member

adriaanm commented Jul 15, 2015

Provide deprecated compatibility stubs for the types and static members,
which forward as follows:

scala.concurrent.forkjoin.ForkJoinPool         => java.util.concurrent.ForkJoinPool
scala.concurrent.forkjoin.ForkJoinTask         => java.util.concurrent.ForkJoinTask
scala.concurrent.forkjoin.ForkJoinWorkerThread => java.util.concurrent.ForkJoinWorkerThread
scala.concurrent.forkjoin.LinkedTransferQueue  => java.util.concurrent.LinkedTransferQueue
scala.concurrent.forkjoin.RecursiveAction      => java.util.concurrent.RecursiveAction
scala.concurrent.forkjoin.RecursiveTask        => java.util.concurrent.RecursiveTask
scala.concurrent.forkjoin.ThreadLocalRandom    => java.util.concurrent.ThreadLocalRandom

To prepare for Java 9, the Scala library avoids using sun.misc.Unsafe.
However, for now, it does provide a convenience accessor for it via scala.concurrent.util.Unsafe.
This (deprecated) class will be removed as soon as the eco-system drops its use
(akka-actor, I'm looking at you -- cc @viktorklang).

@scala-jenkins scala-jenkins added this to the 2.12.0-M3 milestone Jul 15, 2015

@adriaanm

This comment has been minimized.

Show comment
Hide comment
Remove our fork of forkjoin. Java 8 bundles it.
Provide deprecated compatibility stubs for the types and static members,
which forward as follows:

```
scala.concurrent.forkjoin.ForkJoinPool         => java.util.concurrent.ForkJoinPool
scala.concurrent.forkjoin.ForkJoinTask         => java.util.concurrent.ForkJoinTask
scala.concurrent.forkjoin.ForkJoinWorkerThread => java.util.concurrent.ForkJoinWorkerThread
scala.concurrent.forkjoin.LinkedTransferQueue  => java.util.concurrent.LinkedTransferQueue
scala.concurrent.forkjoin.RecursiveAction      => java.util.concurrent.RecursiveAction
scala.concurrent.forkjoin.RecursiveTask        => java.util.concurrent.RecursiveTask
scala.concurrent.forkjoin.ThreadLocalRandom    => java.util.concurrent.ThreadLocalRandom
```

To prepare for Java 9, the Scala library does not itself use `sun.misc.Unsafe`.
However, for now, it provide a convenience accessor for it
via `scala.concurrent.util.Unsafe`. This (deprecated) class will
be removed as soon as the eco-system drops its use
(akka-actor, I'm looking at you).
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 15, 2015

Member

Review by @SethTisue

Member

adriaanm commented Jul 15, 2015

Review by @SethTisue

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jul 15, 2015

Member

This patch will make the Oracle guys working on Project Jigsaw very happy, as it removes the Unsafe dependency 👍

https://issues.scala-lang.org/browse/SI-9381

Member

VladUreche commented Jul 15, 2015

This patch will make the Oracle guys working on Project Jigsaw very happy, as it removes the Unsafe dependency 👍

https://issues.scala-lang.org/browse/SI-9381

@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang
Contributor

viktorklang commented Jul 16, 2015

Thanks @ktoso for akka/akka#18009

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym
Member

retronym commented Jul 16, 2015

Remove further references to forkjoin
Use j.u.c.Forkjoin directly in active and disabled tests

Remove bitrotted benchmarks code

I was going to update these to use `java.util.concurrent.ForkJoin`
directly, instead of our deprecated stubs.

But most of them don't compile anymore (e.g. scala.testing.Benchmark
has been removed, ClassTag imports missing).

While I'm all for benchmarks, we should have large swathes of code
checked in that isn't at compiled and run automatically.

I'm happy to help someone resurrect these in a suitable form.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 16, 2015

Member

Thanks! Looks like I missed quite a few spots :-) Squashed them onto my first round of sweeping.

Member

adriaanm commented Jul 16, 2015

Thanks! Looks like I missed quite a few spots :-) Squashed them onto my first round of sweeping.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 16, 2015

Member

Not sure quite how to manage the updates for these, but I've grepped for forkjoin in the default branch of a few GitHub orgs to show the spots that ought be updated eventually: https://gist.github.com/retronym/979da8069e04fd14b448

Maybe we can find some additional defacto API that needs to be stubbed in these, too.

Member

retronym commented Jul 16, 2015

Not sure quite how to manage the updates for these, but I've grepped for forkjoin in the default branch of a few GitHub orgs to show the spots that ought be updated eventually: https://gist.github.com/retronym/979da8069e04fd14b448

Maybe we can find some additional defacto API that needs to be stubbed in these, too.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 17, 2015

Member

LGTM, I've added the "release-notes" label.

Member

retronym commented Jul 17, 2015

LGTM, I've added the "release-notes" label.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 19, 2015

Member

LGTM

Member

retronym commented Jul 19, 2015

LGTM

SethTisue added a commit that referenced this pull request Jul 20, 2015

Merge pull request #4629 from adriaanm/unforkjoin
Remove our fork of forkjoin. Java 8 bundles it.

@SethTisue SethTisue merged commit 100a234 into scala:2.12.x Jul 20, 2015

6 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [215] SUCCESS. Took 14 s.
Details
validate-main [270] SUCCESS. Took 125 min.
Details
validate-publish-core [272] SUCCESS. Took 12 min.
Details
validate-test [217] SUCCESS. Took 110 min.
Details
@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Jul 20, 2015

Member

@axel22 and @Ichoran, the removal of benchmark code might interest you

Member

SethTisue commented Jul 20, 2015

@axel22 and @Ichoran, the removal of benchmark code might interest you

// TODO: remove once akka no longer needs it, hopefully by 2.12.0-M3!
@Deprecated
public final class Unsafe {

This comment has been minimized.

@axel22

axel22 Jul 20, 2015

Member

There are a couple of other places where we'll need to get rid of Unsafe, most notably, Futures and TrieMap.

@axel22

axel22 Jul 20, 2015

Member

There are a couple of other places where we'll need to get rid of Unsafe, most notably, Futures and TrieMap.

This comment has been minimized.

@viktorklang

viktorklang Jul 20, 2015

Contributor

Futures don't use Unsafe anymore AFAIK

@viktorklang

viktorklang Jul 20, 2015

Contributor

Futures don't use Unsafe anymore AFAIK

This comment has been minimized.

@axel22

axel22 Jul 20, 2015

Member

One left to fix then.

@axel22

axel22 Jul 20, 2015

Member

One left to fix then.

This comment has been minimized.

@axel22

axel22 Jul 21, 2015

Member

Correction - the TrieMap uses AtomicReferenceFieldUpdaters - we never changed it to Unsafe. So, we're good there too.

@axel22

axel22 Jul 21, 2015

Member

Correction - the TrieMap uses AtomicReferenceFieldUpdaters - we never changed it to Unsafe. So, we're good there too.

@axel22

This comment has been minimized.

Show comment
Hide comment
@axel22

axel22 Jul 20, 2015

Member

I'm a little bit surprised that we decided to remove the old (currently unused) benchmark code as part of this PR (which is about dropping our bundled Fork/Join pools) - I thought that PRs are supposed to be on-topic. But that's ok with me.

For what it's worth, I would be interested in reviving some of these benchmarks, and making sure that they run on a nightly basis, along with producing some fancy reports. These need not impact the correctness of the build, but would be useful as a sanity check - we often ask our submitters to do some benchmarks. We certainly have the infrastructure to do that systematically now.

Member

axel22 commented Jul 20, 2015

I'm a little bit surprised that we decided to remove the old (currently unused) benchmark code as part of this PR (which is about dropping our bundled Fork/Join pools) - I thought that PRs are supposed to be on-topic. But that's ok with me.

For what it's worth, I would be interested in reviving some of these benchmarks, and making sure that they run on a nightly basis, along with producing some fancy reports. These need not impact the correctness of the build, but would be useful as a sanity check - we often ask our submitters to do some benchmarks. We certainly have the infrastructure to do that systematically now.

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