RFR: 8087322: Implement a Semaphore utility class

[Kim Barrett]

On Jun 18, 2015, at 9:27 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Stefan,
> 
> Sorry for the delay …

Stefan and I have been discussing various different approaches in separate email.  I think I’ve received 5 (substantially) different variants from him, and offered him one of my own so far.  I’m hopeful that we’re approaching agreement though.  And hopefully we’ll end up in a place that will look good to the rest of you too.

> One thing I am tempted to clean up a little more is the timedwait call.

I think that can likely be made common to all non-Apple posix systems, at least for those Oracle supports or that otherwise exist in the open repository.  All those posix variants appear to support clock_gettime of CLOCK_REALTIME, at least according to their documentation.  Certainly the minimum Oracle-supported Linux and Solaris versions have it (and at least for Linux, have for a long time).  Both BSD and AIX documentation put it in their libc, so we don’t even need to update the libraries to link against.  So I think we can also unify the time access for all of the non-Apple posix variants.  If someone is porting to some other “posix” platform that doesn’t supply clock_gettime, they’ll need to do some additional work, and we might end up needing to tweak the modularity a bit to support that, but I’m inclined to wait for such a request to actually be made.

> The sr_semaphore appears to be being initialized by C++ static initialization, which is a concern for me, primarily because Semaphore construction requires allocation support (it's a CHeapObj) and utilizes NMT. I get very nervous when NMT gets involved prior to VM initialization. Can we make sr_semaphore a pointer instead and initialize it in one of the os::init methods?

I think that as long as we’re not allocating any CHeapObj subobjects, that isn’t a problem.  The fact that it is a CHeapObj permits, but does not require, allocation using the CHeapObj allocator.  At least, I think that’s true.  Certainly it won’t be allocated using its associated operator new, so I don’t see how NMT could get involved.

> 1069     } else {
> 1070       return false;
> 1071     }
> 
> This is returning false when an error has occurred. At a minimum there should be an assert(false, "...") so that we detect this in debug builds.

One of the things I worked on in the version I gave to Stefan was being pedantic about error checking.

> Can we not implement this:
> 
> 53   void signal();
> 54   void signal(uint count);
> 
> as simply:
> 
>  void signal(uint count = 1);

I think it ends up being more convenient in the implementation to have them be separate.  If that ultimately turns out not to be the case then we can change it.  It doesn’t affect the client API at all (unless someone starts using pointers to member functions).