RFR: 8145725: Remove the WorkAroundNPTLTimedWaitHang workaround
David Holmes
david.holmes at oracle.com
Wed Feb 10 11:10:58 UTC 2016
Hi Thomas,
On 10/02/2016 7:32 PM, Thomas Stüfe wrote:
> Hi David,
>
> I took a look at this, this is a nice cleanup.
>
> One question, in Parker::unpark, you moved the comparison (s<1) outside
> the mutex lock scope. The pthread_cond_signal() call was already outside
> the scope.
>
> Would it make not sense to move the comparison and conditional
> pthread_cond_signal into the mutex scope?
The signalling is deliberately done outside the mutex scope as it is
more performant. This is the code we have been executing in the else
part of the "if (WorkAroundNPTLTimedWaitHang) ..."
The move of the s<1 was not intentionally done, but s is a local
variable and so the change is not affected by the mutex.
Thanks,
David
> Kind Regards, Thomas
>
>
>
> On Wed, Feb 10, 2016 at 5:41 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Thanks Dmitry!
>
> Can I have a second Reviewer please.
>
> David
>
>
> On 9/02/2016 6:06 PM, Dmitry Dmitriev wrote:
>
> Hello David,
>
> Looks good to me!
>
> Thanks,
> Dmitry
>
> On 09.02.2016 4:17, David Holmes wrote:
>
> Hi Dmitry,
>
> Great cleanup suggestions. I also did some missing assert ->
> assert_status conversions for the pthread_* functions. (And
> reverted a
> change that shouldn't have been made - assert_status is for
> when the
> "status" is the error code.)
>
> New webrev:
> http://cr.openjdk.java.net/~dholmes/8145725/webrev.v2/
>
> Thanks,
> David
> -----
>
> On 8/02/2016 11:24 PM, Dmitry Dmitriev wrote:
>
> Hello David,
>
> Looks good to me. Just few additional clean-ups which
> you can consider
> for implementing.
> 1) hotspot/src/os/bsd/vm/os_bsd.cpp module
>
> I think that you can remove duplicated logic for mutex
> unlock from
> Parker::unpark function.
>
> I.e. transform it from:
> 4376 void Parker::unpark() {
> ...
> 4381 if (s < 1) {
> 4382 status = pthread_mutex_unlock(_mutex);
> 4383 assert_status(status == 0, status, "invariant");
> 4384 status = pthread_cond_signal(_cond);
> 4385 assert_status(status == 0, status, "invariant");
> 4386 } else {
> 4387 pthread_mutex_unlock(_mutex);
> 4388 assert_status(status == 0, status, "invariant");
> 4389 }
> 4390 }
>
> To something like that:
> void Parker::unpark() {
> ...
> status = pthread_mutex_unlock(_mutex);
> assert_status(status == 0, status, "invariant");
> if (s < 1) {
> status = pthread_cond_signal(_cond);
> assert_status(status == 0, status, "invariant");
> }
> }
>
> 2) hotspot/src/os/linux/vm/os_linux.cpp
>
> Similar refactoring for Parker::unpark for removing
> duplicated logic for
> mutex unlock.
>
> From:
> 5720 void Parker::unpark() {
> ...
> 5725 if (s < 1) {
> 5726 // thread might be parked
> 5727 if (_cur_index != -1) {
> 5728 // thread is definitely parked
> 5729 // must capture correct index before unlocking
> 5730 int index = _cur_index;
> 5731 status = pthread_mutex_unlock(_mutex);
> 5732 assert(status == 0, "invariant");
> 5733 status = pthread_cond_signal(&_cond[index]);
> 5734 assert(status == 0, "invariant");
> 5735 } else {
> 5736 pthread_mutex_unlock(_mutex);
> 5737 assert(status == 0, "invariant");
> 5738 }
> 5739 } else {
> 5740 pthread_mutex_unlock(_mutex);
> 5741 assert(status == 0, "invariant");
> 5742 }
> 5743 }
>
> To:
>
> void Parker::unpark() {
> ...
> if ((s < 1) && (_cur_index != -1)) {
> // thread is definitely parked
> // must capture correct index before unlocking
> int index = _cur_index;
> status = pthread_mutex_unlock(_mutex);
> assert(status == 0, "invariant");
> status = pthread_cond_signal(&_cond[index]);
> assert(status == 0, "invariant");
> } else {
> pthread_mutex_unlock(_mutex);
> assert(status == 0, "invariant");
> }
> }
>
>
> Thanks,
> Dmitry
>
> On 08.02.2016 10:16, David Holmes wrote:
>
> webrev:
> http://cr.openjdk.java.net/~dholmes/8145725/webrev/
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8145725
>
> In JDK-8130728 we disabled the workaround by default
> and have not seen
> any issues (other than JDK-8029453). So now the
> workaround can be
> removed completely.
>
> Thanks,
> David
>
>
>
>
More information about the hotspot-runtime-dev
mailing list