RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Thu, Nov 17, 2016 at 12:03 PM, Paul Sandoz <Paul.Sandoz at oracle.com>
wrote:

>
> Perhaps consolidate the repeated mini bit set methods as package private
> static methods on AbstractCollection? Unclear where the j.u.concurrent ones
> should go though...
>

Deferred.


> It’s purely a subjective thing but i would be inclined to change the
> variable name “deathRow" to something more neutral such as “removedBitSet”.
>

It's certainly evocative!  Especially given that they may get a reprieve
from a following predicate!


> The 2 layer nested loop is quite clever, certainly twists the mind when
> first encountered. It’s arguably more readable if there are two separate,
> (not-nested) loops, but that also requires two explicit invocations of the
> action, which may perturb the performance characteristics.
>

After you've written a hundred of these loops, they start to look rather
natural/idiomatic.  I think these nested loops should be the canonical way
to implement traversals of circular buffers in any programming language.
The inner loops seem optimal.  It seems unlikely we've invented something
new here, but maybe...


> ArrayDeque
> —
>
>  188     public ArrayDeque(int numElements) {
>  189         elements = new Object[Math.max(1, numElements + 1)];
>  190     }
>
> What if Integer.MAX_VALUE is passed?
>

Now tries (and fails!) to allocate a maximal array.


>  202     public ArrayDeque(Collection<? extends E> c) {
>  203         elements = new Object[c.size() + 1];
>  204         addAll(c);
>  205     }
>
> What if c.size() returns Integer.MAX_VALUE?
>
>
Now delegates to the other constructor.


>
>  226      * Adds i and j, mod modulus.
>  227      * Precondition and postcondition: 0 <= i < modulus, 0 <= j <=
> modulus.
>  228      */
>  229     static final int add(int i, int j, int modulus) {
>
> Is that upper bound correct for j? 0 <= j < modulus
>

j renamed to distance, to emphasize asymmetry.
Although in this file, distance will never equal modulus.


>
>  317         c.forEach(e -> addLast(e));
>
> this::addLast, up to you which you prefer
>
>
Meh.  Left as is; another vote could tip to the other side.


>
>  843         public boolean tryAdvance(Consumer<? super E> action) {
>  844             if (action == null)
>  845                 throw new NullPointerException();
>  846             int t, i;
>  847             if ((t = fence) < 0) t = getFence();
>
> Is that for optimisation purposes, since the same check is also performed
> in getFence? If so that seems like overkill
>
>
done.


>
>  848             if (t == (i = cursor))
>  849                 return false;
>  850             final Object[] es;
>  851             action.accept(nonNullElementAt(es = elements, i));
>  852             cursor = inc(i, es.length);
>
> Recommend updating cursor before the action
>
>
done.


>
>  853             return true;
>  854         }
>
>
>
> Collection8Test
> —
>
>  429     public void testRemoveAfterForEachRemaining() {
>
> Are tests run by default with testImplementationDetails == true? I am
> guessing so.
>
>
We run various combinations in jtreg mode.

 * @run junit/othervm/timeout=1000 -Djsr166.testImplementationDetails=true
JSR166TestCase
 * @run junit/othervm/timeout=1000
-Djava.util.concurrent.ForkJoinPool.common.parallelism=0
-Djsr166.testImplementationDetails=true JSR166TestCase
 * @run junit/othervm/timeout=1000
-Djava.util.concurrent.ForkJoinPool.common.parallelism=1
-Djava.util.secureRandomSeed=true JSR166TestCase

The original target for these tests are the jck, and that is expected to
run with the default testImplementationDetails=false