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 >>>>>>>>>>>>>> ----- >>>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>> >>>> >>
