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 <[email protected]> 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?
