RFR: jsr166 jdk9 integration wave 7

[Paul Sandoz]

Hi,

This first the part of my review that concentrates mostly on the implementation.

Minor stuff. I don’t claim to know enough about the changes in FJ/CF, so i especially focused on the VH changes for those classes.

Paul.

CompletableFutureTest
—

3383     public void testRejectingExecutor() {
3384         for (Integer v : new Integer[] { 1, null }) {
3385
3386         final CountingRejectingExecutor e = new CountingRejectingExecutor();


3473     public void testRejectingExecutorNeverInvoked() {
3474         final CountingRejectingExecutor e = new CountingRejectingExecutor();
3475
3476         for (Integer v : new Integer[] { 1, null }) {
3477
3478         final CompletableFuture<Integer> complete = CompletableFuture.completedFuture(v);

No indent for code within the for loop block


ForkJoin
—

 571      * Style notes
 572      * ===========
 573      *
 574      * Memory ordering relies mainly on VarHandles.  This can be
 575      * awkward and ugly, but also reflects the need to control
 576      * outcomes across the unusual cases that arise in very racy code
 577      * with very few invariants. So these explicit checks would exist
 578      * in some form anyway.  All fields are read into locals before
 579      * use, and null-checked if they are references.  This is usually
 580      * done in a "C"-like style of listing declarations at the heads
 581      * of methods or blocks, and using inline assignments on first
 582      * encounter.  Nearly all explicit checks lead to bypass/return,
 583      * not exception throws, because they may legitimately arise due
 584      * to cancellation/revocation during shutdown.

The paragraph is referring to explicit checks (null and bounds checks) that were removed when the reference to Unsafe was removed.


2257      * @param saturate if nonnull, a predicate invoked upon attempts

s/nonnull/non-null



StampedLock
—

I notice lots of use of @ReservedStackAccess (same for ReentrantReadWriteLock), including Fred who might wanna look too.


ConcurrentLinkedDeque
—

linkFirst uses HEAD.compareAndSet, where as linkLast uses TAIL.weakCompareAndSetVolatile, can the former use the weak variant too since, as commented, failure is OK.


AtomicInteger
—

 185     public final int incrementAndGet() {
 186         return (int)VALUE.getAndAdd(this, 1) + 1;
 187     }


You can use VALUE.addAndGet same for other similar methods here and in other classes.




> On 27 Jun 2016, at 21:38, Martin Buchholz <martinrb at google.com> wrote:
> 
> jsr166 has been pervasively modified to use VarHandles, but there are some other pending changes (that cannot be cleanly separated from VarHandle conversion).  We expect this to be the last feature integration for jdk9.
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
> 
> We're asking Paul to do most of the review work here, as owner of VarHandles JEP and as Oracle employee.
> We need approval for API changes in
> 
> https://bugs.openjdk.java.net/browse/JDK-8157523
> Various improvements to ForkJoin/SubmissionPublisher code
> 
> https://bugs.openjdk.java.net/browse/JDK-8080603
> Replace Unsafe with VarHandle in java.util.concurrent classes
> 
> There is currently a VarHandle bootstrap problem with ThreadLocal/AtomicInteger that causes
> java/util/Locale/Bug4152725.java
> to fail.  Again I'm hoping that Paul will figure out what to do.  In the past rearranging the order of operations in <clinit> has worked for similar problems.  It's not surprising we have problems, since j.u.c. needs VarHandles initialized to do work, and j.l.invoke needs j.u.c. (notably AtomicInteger and ConcurrentHashMap) to do work.  Maybe we need some very low-level concurrency infrastructure that does not use VarHandles, only for bootstrap?