RFR: jsr166 jdk9 integration wave 12 (original) (raw)

Martin Buchholz martinrb at google.com
Fri Nov 18 04:29:12 UTC 2016

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.MAXVALUE 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.MAXVALUE? 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.

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

More information about the core-libs-dev mailing list