RFR: 8011882: Replace spin loops as back off when suspending

[Rickard Bäckman]

David,

thanks for the review.

/R

On Apr 22, 2013, at 8:23 AM, David Holmes wrote:

> Okay - seems ready to ship. :)
> 
> Thanks,
> David
> 
> On 22/04/2013 3:20 PM, Rickard Bäckman wrote:
>> David,
>> 
>> you are right, the only users of this mechanism are the old flatprofiler (which from what I could figure runs from the WatcherThread) and our sampler mechanism.
>> That one also runs from the WatcherThread. The point of the assert you are writing about is to make sure that we actually consumed any post that the suspended thread
>> may have issued.
>> 
>> Thanks
>> /R
>> 
>> On Apr 22, 2013, at 1:53 AM, David Holmes wrote:
>> 
>>> Hi Rickard,
>>> 
>>> On 20/04/2013 12:19 AM, Rickard Bäckman wrote:
>>>> David,
>>>> 
>>>> here is an updated webrev with the changes incorporated.
>>>> 
>>>> http://cr.openjdk.java.net/~rbackman/8011882.1/
>>> 
>>> The changes look reasonable.
>>> 
>>> My only concern is the:
>>> 
>>> assert(!sr_semaphore.trywait(), "semaphore has invalid state");
>>> 
>>> I'm not completely clear on the higher-level protocols and usage of this API and what actions can be attempted concurrently. These asserts indicate a strict one thread at a time usage - is that right?
>>> 
>>> The validation of all this comes in the testing of course.
>>> 
>>> Thanks,
>>> David
>>> 
>>>> Thanks
>>>> /R
>>>> 
>>>> On Apr 16, 2013, at 9:44 AM, Rickard Bäckman wrote:
>>>> 
>>>>> David,
>>>>> 
>>>>> thanks for the input, I'll go back to the split versions and update the timing.
>>>>> 
>>>>> /R
>>>>> 
>>>>> On Apr 16, 2013, at 1:27 AM, David Holmes wrote:
>>>>> 
>>>>>> PS. Also see the existing unpackTime and compute_abstime helper functions for dealing with pthread/POSIX absolute timed-waits. Better than using javaTimeMillis()
>>>>>> 
>>>>>> David
>>>>>> 
>>>>>> On 15/04/2013 10:50 PM, David Holmes wrote:
>>>>>>> On 15/04/2013 10:07 PM, Rickard Bäckman wrote:
>>>>>>>> David,
>>>>>>>> 
>>>>>>>> this is what the suggested semaphore.cpp/semaphore.hpp. Is that what
>>>>>>>> you are looking for?
>>>>>>> 
>>>>>>> <sigh> I thought so till I saw it - far uglier and complicated than I
>>>>>>> had hoped. Sadly the three separate versions wins for me.
>>>>>>> 
>>>>>>> By the way you can't do this:
>>>>>>> 
>>>>>>> 116 bool Semaphore::timedwait(unsigned int sec, int nsec) {
>>>>>>> 117   struct timespec ts;
>>>>>>> 118   jlong endtime = os::javaTimeNanos() + (sec * NANOSECS_PER_SEC) +
>>>>>>> nsec;
>>>>>>> 119   ts.tv_sec = endtime / NANOSECS_PER_SEC;
>>>>>>> 120   ts.tv_nsec = endtime % NANOSECS_PER_SEC;
>>>>>>> 
>>>>>>> javaTimeNanos is not wall-clock time, but the POSIX sem_timewait
>>>>>>> requires an absolute time - you need to use javaTimeMillis(). Which also
>>>>>>> means the wait will be affected by changes to wall-clock time.
>>>>>>> 
>>>>>>> David
>>>>>>> -----
>>>>>>> 
>>>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/webrev/
>>>>>>>> 
>>>>>>>> Thanks
>>>>>>>> /R
>>>>>>>> 
>>>>>>>> On Apr 15, 2013, at 8:59 AM, David Holmes wrote:
>>>>>>>> 
>>>>>>>>> On 15/04/2013 4:55 PM, Rickard Bäckman wrote:
>>>>>>>>>> David,
>>>>>>>>>> 
>>>>>>>>>> any new thoughts?
>>>>>>>>> 
>>>>>>>>> Not a new one but I think factoring into Semaphore.hpp/cpp and using
>>>>>>>>> a few ifdefs is better than three versions of the Semaphore class.
>>>>>>>>> The signal thread could use it also.
>>>>>>>>> 
>>>>>>>>> David
>>>>>>>>> 
>>>>>>>>>> Thanks
>>>>>>>>>> /R
>>>>>>>>>> 
>>>>>>>>>> On Apr 12, 2013, at 8:06 AM, Rickard Bäckman wrote:
>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> On Apr 12, 2013, at 7:34 AM, David Holmes wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> On 12/04/2013 3:01 PM, Rickard Bäckman wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> On Apr 12, 2013, at 1:04 AM, David Holmes wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> On 11/04/2013 11:02 PM, Rickard Bäckman wrote:
>>>>>>>>>>>>>>> On Apr 11, 2013, at 2:39 PM, David Holmes wrote:
>>>>>>>>>>>>>>>> So what did you mean about pthread_semaphore (what is that
>>>>>>>>>>>>>>>> anyway?) ??
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Never mind, pthread condition variables.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Ah I see.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> I really, really, really don't like seeing three versions of
>>>>>>>>>>>>>>>> this class :( Can't BSD and Linux at least share a POSIX
>>>>>>>>>>>>>>>> version? (And I wonder if we can actually mix-n-match UI
>>>>>>>>>>>>>>>> threads on Solaris with POSIX semaphores on Solaris?)
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> I don't like it either, our OS code isn't really helpful when
>>>>>>>>>>>>>>> it comes do reusing things :) Not sure how I would layout
>>>>>>>>>>>>>>> things to make them only available on BSD (Not Mac) and Linux.
>>>>>>>>>>>>>>> I guess os_posix.hpp with lots of #ifdefs, but I'm not sure I"m
>>>>>>>>>>>>>>> feeling that happy about that.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Why would the os_posix version need a lot of ifdefs?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Well, I guess we would need:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> (in ifdef pseudo language)
>>>>>>>>>>>>> 
>>>>>>>>>>>>> #ifdef (LINUX || (BSD && !APPLE))
>>>>>>>>>>>>> …
>>>>>>>>>>>>> #endif
>>>>>>>>>>>> 
>>>>>>>>>>>> But if it isn't "posix" then we won't be building os_posix - right?
>>>>>>>>>>> 
>>>>>>>>>>> Linux, Solaris, Bsd & Mac builds and include os_posix. They are all
>>>>>>>>>>> "implementing posix" we are just not using the same semaphore
>>>>>>>>>>> implementation on all of them.
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>>> The second interesting problem this will get us into is that
>>>>>>>>>>>>> sem_t is not declared in this context. Where do we put the
>>>>>>>>>>>>> #include <semaphore.h>? Impossible in os_posix.hpp since it is
>>>>>>>>>>>>> included in the middle of a class definition. I could put it in
>>>>>>>>>>>>> os.hpp in the #ifdef path that does the jvm_platform.h includes,
>>>>>>>>>>>>> not sure if that is very pretty either.
>>>>>>>>>>>> 
>>>>>>>>>>>> Semaphores are already used by the signal handler thread -
>>>>>>>>>>>> semaphore.h is included in os_linux.cpp etc, so why would os_posix
>>>>>>>>>>>> be any different ?
>>>>>>>>>>>> 
>>>>>>>>>>>> But couldn't we just have a Semaphore.h/cpp with any needed ifdefs?
>>>>>>>>>>>> 
>>>>>>>>>>>>>> Do we really have four versions:
>>>>>>>>>>>>>> - linux (posix)
>>>>>>>>>>>>>> - BSD (posix)
>>>>>>>>>>>>>> - Solaris
>>>>>>>>>>>>>> - Mac (different to BSD?)
>>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 3:
>>>>>>>>>>>>> 1) linux & bsd uses the sem_ interface
>>>>>>>>>>>>> 2) solaris uses the sema_ interface
>>>>>>>>>>>>> 3) mac uses the semaphore_ interface
>>>>>>>>>>>> 
>>>>>>>>>>>> Okay but if mac is BSD why can't we use bsd ie posix interface
>>>>>>>>>>>> instead of the mach semaphore_ ?
>>>>>>>>>>> 
>>>>>>>>>>> Because apple decided not to implement sem_timedwait.
>>>>>>>>>>> On Solaris we use sema_ because sem_ requires us to link with -lrt
>>>>>>>>>>> which we currently don't (and I'm not really feeling like adding it)
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> BTW I like the idea of using the semaphore, we're just haggling on
>>>>>>>>>>>> the details. ;-)
>>>>>>>>>>> 
>>>>>>>>>>> I'm fine with that :)
>>>>>>>>>>> 
>>>>>>>>>>> /R
>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>> 
>>>>>>>>>>>>> /R
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> ??
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>> -----
>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>> 
>>>>> 
>>>> 
>>