Re: RFR: jsr166 jdk9 integration wave 10

2016-09-12 Thread Aleksey Shipilev 
On 09/12/2016 07:57 PM, Martin Buchholz wrote:
> is better spelled like this?
> 
>  176  * setPendingCount(1); // only one pending, not two!
> 
> I already struggled over the wording here  hmmm ... how about
> 
> // looks off by one, but correct!

Yes, this is better.

> I don't like the verbosity of that.  Probably something closer to
> Guava's Stopwatch would be cleaner. ... But that's a big TODO.

Ok, fine.

Thanks,
-Aleksey

Re: RFR: jsr166 jdk9 integration wave 10

2016-09-12 Thread Paul Sandoz 

> On 12 Sep 2016, at 09:57, Martin Buchholz  wrote:
> 
> 
> 
> On Mon, Sep 12, 2016 at 3:22 AM, Aleksey Shipilev  > wrote:
> 
> Minor things:
> 
> CountedCompleter.java:
> 
>  176  * setPendingCount(1); // not off by one !
> 
> is better spelled like this?
> 
>  176  * setPendingCount(1); // only one pending, not two!
> 
> I already struggled over the wording here  hmmm ... how about
> 
> // looks off by one, but correct!
> 

Ok, with me.

Paul.

Re: RFR: jsr166 jdk9 integration wave 10

2016-09-12 Thread Martin Buchholz 
On Mon, Sep 12, 2016 at 3:22 AM, Aleksey Shipilev  wrote:

>
> Minor things:
>
> CountedCompleter.java:
>
>  176  * setPendingCount(1); // not off by one !
>
> is better spelled like this?
>
>  176  * setPendingCount(1); // only one pending, not two!
>

I already struggled over the wording here  hmmm ... how about

// looks off by one, but correct!


>
> SubmissionTest.java:
>
> +static long millisElapsedSince(long startTime) {
> +return (System.nanoTime() - startTime) / (1000L * 1000L);
> +}
> ...
> +if (millisElapsedSince(startTime) >= LONG_DELAY_MS) {
>
> I hate these unit conversions sprinkled everywhere. Can it be like this?
>
>
This is already used pervasively in the tck tests.  Imagine
that millisElapsedSince were moved to a utility class, like JSR166TestCase,
but for jtreg tests.


>  if (TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) >=
> LONG_DELAY_MS)
>

 I don't like the verbosity of that.  Probably something closer to Guava's
Stopwatch would be cleaner.
http://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Stopwatch.html
But that's a big TODO.

Re: RFR: jsr166 jdk9 integration wave 10

2016-09-12 Thread Paul Sandoz 

> On 10 Sep 2016, at 10:21, Martin Buchholz  wrote:
> 
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
> 

+1

CountedCompleter
—

 176  * setPendingCount(1); // not off by one !


CountedCompleterTest
—

1910 setPendingCount(1); // not off by one !


I cannot resist saying you have an off by one (letter) error :-)



> Mostly docs and tests.
> 
> jsr166 CVS is now caught up with jdk9+135
> 
> Paul, I think we need to update AtomicInteger for VarHandle#getAndAdd:
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/miscellaneous/src/java.base/share/classes/java/util/concurrent/atomic/AtomicInteger.java.udiff.html
> 
> 

Oops, sorry about that, i missed ‘em.

Paul.

Re: RFR: jsr166 jdk9 integration wave 10

2016-09-12 Thread Aleksey Shipilev 
On 09/10/2016 08:21 PM, Martin Buchholz wrote:
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
> 
> Mostly docs and tests.

Looks good. Very nice use of (frequently overlooked) local classes in
examples.

Minor things:

CountedCompleter.java:

 176  * setPendingCount(1); // not off by one !

is better spelled like this?

 176  * setPendingCount(1); // only one pending, not two!


SubmissionTest.java:

+static long millisElapsedSince(long startTime) {
+return (System.nanoTime() - startTime) / (1000L * 1000L);
+}
...
+if (millisElapsedSince(startTime) >= LONG_DELAY_MS) {

I hate these unit conversions sprinkled everywhere. Can it be like this?

 if (TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) >=
LONG_DELAY_MS)

Thanks,
-Aleksey

RFR: jsr166 jdk9 integration wave 10

2016-09-10 Thread Martin Buchholz 
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

Mostly docs and tests.

jsr166 CVS is now caught up with jdk9+135

Paul, I think we need to update AtomicInteger for VarHandle#getAndAdd:

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/miscellaneous/src/java.base/share/classes/java/util/concurrent/atomic/AtomicInteger.java.udiff.html