RFR: jsr166 jdk10 integration wave 5

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: jsr166 jdk10 integration wave 5

Martin Buchholz-3
The notable thing this time around is the embarrassing number of rare races
being fixed, all of which are second tries.  This time for sure!

There's a large number of boring changes to appease errorprone, notably
http://errorprone.info/bugpattern/RandomModInteger
http://errorprone.info/bugpattern/MixedArrayDimensions

http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/overview.html

8190747: ExecutorService/Invoke.java fails intermittently
8179314: CountedCompleterTest.testForkHelpQuiesce fails with expected:<21>
but was:<13>
8189387: ConcurrentLinkedDeque linearizability continued ...
8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

David Holmes
Hi Martin,

On 7/11/2017 6:00 AM, Martin Buchholz wrote:

> The notable thing this time around is the embarrassing number of rare
> races being fixed, all of which are second tries.  This time for sure!
>
> There's a large number of boring changes to appease errorprone, notably
> http://errorprone.info/bugpattern/RandomModInteger
> http://errorprone.info/bugpattern/MixedArrayDimensions
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/overview.html
>
> 8190747: ExecutorService/Invoke.java fails intermittently

Looks fine.

> 8179314: CountedCompleterTest.testForkHelpQuiesce fails with
> expected:<21> but was:<13>

Looks fine.

> 8189387: ConcurrentLinkedDeque linearizability continued ...

Can't really comment on linearizability changes but I found  this change
to be very confusing:

src/java.base/share/classes/java/util/concurrent/ConcurrentLinkedDeque.java

       final Node<E> pred(Node<E> p) {
!         Node<E> q = p.prev;
!         return (p == q) ? last() : q;
       }

       /**
        * Returns the first node, the unique node p for which:
        *     p.prev == null && p.next != p
--- 693,705 ----
        * Returns the predecessor of p, or the last node if p.prev has been
        * linked to self, which will only be true if traversing with a
        * stale pointer that is now off the list.
        */
       final Node<E> pred(Node<E> p) {
!         if (p == (p = p.prev))
!             p = last();
!         return p;
       }

The original version is quite clear, the new version is trying to be far
too clever with order of evaluation and to me is far less clear.


> 8189764: Miscellaneous changes imported from jsr166 CVS 2017-11

All seem okay. Though I'm curious about the changes from
"catch(Throwable" to "catch(Exception" ?

Thanks,
David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Martin Buchholz-3
On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <[hidden email]>
wrote:

>
> src/java.base/share/classes/java/util/concurrent/ConcurrentL
> inkedDeque.java
>
>       final Node<E> pred(Node<E> p) {
> !         Node<E> q = p.prev;
> !         return (p == q) ? last() : q;
>       }
>
>       /**
>        * Returns the first node, the unique node p for which:
>        *     p.prev == null && p.next != p
> --- 693,705 ----
>        * Returns the predecessor of p, or the last node if p.prev has been
>        * linked to self, which will only be true if traversing with a
>        * stale pointer that is now off the list.
>        */
>       final Node<E> pred(Node<E> p) {
> !         if (p == (p = p.prev))
> !             p = last();
> !         return p;
>       }
>
> The original version is quite clear, the new version is trying to be far
> too clever with order of evaluation and to me is far less clear.


Well, this idiom is already in widespread use in these files, notably in
succ, which was non-symmetrical with pred for no reason.  The trickier code
saves 2 bytes of bytecode; admittedly unlikely to make a difference; we are
not near the 35-bytecode limit.  The more common form of the idiom is

                if (p == (p = p.next))
                    continue restart;

After 10 years working on lock-free queues it starts to look natural (but
don't try it in C++!)
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Martin Buchholz-3
In reply to this post by David Holmes
Thanks for the review!

On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <[hidden email]>
wrote:

>
> 8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
>>
>
> All seem okay. Though I'm curious about the changes from "catch(Throwable"
> to "catch(Exception" ?
>

There's a half-hearted attempt to appease
http://errorprone.info/bugpattern/TryFailThrowable
No actual bugs were fixed.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

David Holmes
On 7/11/2017 8:17 AM, Martin Buchholz wrote:

> Thanks for the review!
>
> On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>         8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
>
>
>     All seem okay. Though I'm curious about the changes from
>     "catch(Throwable" to "catch(Exception" ?
>
>
> There's a half-hearted attempt to appease
> http://errorprone.info/bugpattern/TryFailThrowable
> No actual bugs were fixed.

Hmmm. Not sure I see a problem if threadUnexpectedException just wraps
and rethrows the original exception. On the other hand Errors are no
longer being handled this way. Probably doesn't make a difference either
way.

Thanks,
David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Martin Buchholz-3
On Mon, Nov 6, 2017 at 5:33 PM, David Holmes <[hidden email]>
wrote:

> On 7/11/2017 8:17 AM, Martin Buchholz wrote:
>
>> Thanks for the review!
>>
>> On Mon, Nov 6, 2017 at 1:36 PM, David Holmes <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>
>>         8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
>>
>>
>>     All seem okay. Though I'm curious about the changes from
>>     "catch(Throwable" to "catch(Exception" ?
>>
>>
>> There's a half-hearted attempt to appease
>> http://errorprone.info/bugpattern/TryFailThrowable
>> No actual bugs were fixed.
>>
>
> Hmmm. Not sure I see a problem if threadUnexpectedException just wraps and
> rethrows the original exception. On the other hand Errors are no longer
> being handled this way. Probably doesn't make a difference either way.
>

Yes, probably doesn't make a difference either way.  These  TryFailThrowable
<http://errorprone.info/bugpattern/TryFailThrowable> warnings just don't
have enough value, so they are all reverted back to catch (Throwable).
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Paul Sandoz
In reply to this post by Martin Buchholz-3

> On 6 Nov 2017, at 12:00, Martin Buchholz <[hidden email]> wrote:
>
> The notable thing this time around is the embarrassing number of rare races being fixed, all of which are second tries.  This time for sure!
>
> There's a large number of boring changes to appease errorprone, notably
> http://errorprone.info/bugpattern/RandomModInteger
> http://errorprone.info/bugpattern/MixedArrayDimensions
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/overview.html
>

Looks good.


> 8190747: ExecutorService/Invoke.java fails intermittently
> 8179314: CountedCompleterTest.testForkHelpQuiesce fails with expected:<21> but was:<13>
> 8189387: ConcurrentLinkedDeque linearizability continued …


 685     final Node<E> succ(Node<E> p) {
 686         // TODO: should we skip deleted nodes here?

Is the comment still relevant?

Paul.

> 8189764: Miscellaneous changes imported from jsr166 CVS 2017-11
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Martin Buchholz-3
On Wed, Nov 8, 2017 at 2:09 PM, Paul Sandoz <[hidden email]> wrote:

>
>
>  685     final Node<E> succ(Node<E> p) {
>  686         // TODO: should we skip deleted nodes here?
>
> Is the comment still relevant?
>

It's still an open question, like what's the best GC strategy, or should
ArrayList automatically shrink (as well as grow) its backing array?
Reply | Threaded
Open this post in threaded view
|

Re: RFR: jsr166 jdk10 integration wave 5

Paul Sandoz

> On 8 Nov 2017, at 14:20, Martin Buchholz <[hidden email]> wrote:
>
>
>
> On Wed, Nov 8, 2017 at 2:09 PM, Paul Sandoz <[hidden email]> wrote:
>
>
>  685     final Node<E> succ(Node<E> p) {
>  686         // TODO: should we skip deleted nodes here?
>
> Is the comment still relevant?
>
> It's still an open question, like what's the best GC strategy, or should ArrayList automatically shrink (as well as grow) its backing array?

Ok, i thought it might be somewhat related to skipping over null nodes and thus related to the linearizability fixes.

Paul.