RFR 8005311: Add Scalable Updatable Variables, DoubleAccumulator, DoubleAdder, LongAccumulator, LongAdder

[Remi Forax]

On 01/05/2013 07:10 PM, Chris Hegarty wrote:
> As part of JEP 155 we are proposing to add the following public 
> classes to support Scalable Updatable Variables, DoubleAccumulator, 
> DoubleAdder, LongAccumulator and LongAdder.
>
> These have been written by Doug Lea, with assistance from members of 
> the former JCP JSR-166 Expert Group.
>
> Webrev and javadoc are at:
>   http://cr.openjdk.java.net/~chegar/8005311/ver.00/
>
> Since Doug is the author, I am taking a reviewer/sponsor role.
>
> Here are my initial comments.
>  - There are various places in DoubleAccmulator where there are broken
>    links to #sum ( I think it is just a cut'n'paste error ). These
>    should be #get.
>  - Accumulators
>    {@link #get} may read somewhat better as {@link #get current value} ??
>  - Accumulators
>    Does the 'identity' value need further explanation?
>
> Note: There is one minor change to the implementation. Currently in 
> the jdk8 repo j.u.f.DoubleBinaryOperator defines operateAsDouble. This 
> method has been renamed to applyAsDouble in the lambda/lambda repo. 
> When these changes are sync'ed from lambda/lambda this can be 
> reverted. A similar comment has been added to the code.
>
> -Chris.

The code is not very java-ish,
by example, in Striped64,

Cell[] as; Cell a; int n; long v;
if ((as = cells) != null && (n = as.length) > 0) {
     if ((a = as[(n - 1) & h]) == null) {

instead

int n;
Cell[] as = cells;
if (as != null && (n = as.length) > 0) {
     Cell a = as[(n - 1) & h];
     if ((a == null) {


also I think that the variable created (and 'init' after in the code) 
are not needed.

boolean created = false;
try {               // Recheck under lock
     Cell[] rs; int m, j;
     if ((rs = cells) != null &&
        (m = rs.length) > 0 &&
          rs[j = (m - 1) & h] == null) {
          rs[j] = r;
          created = true;
     }
} finally {
     cellsBusy = 0;
}
if (created)
    break;

The code can become

try {               // Recheck under lock
     Cell[] rs = cells; int m, j;
     if (rs != null &&
        (m = rs.length) > 0 &&
          rs[j = (m - 1) & h] == null) {
          rs[j] = r;
          break;
     }
} finally {
     cellsBusy = 0;
}

Overall, I think there are too many lazy initializations.
Unlike HashMap, if a developer uses let say LongAccumulator it's because 
AtomicLong doesn't work well,
so not having the array of cells initialized by default seems weird.

Rémi