RFR: 8216258: Make FreeIdSet semaphore-based

[Thomas Schatzl]

Hi Kim,

On Tue, 2019-01-08 at 20:25 -0500, Kim Barrett wrote:
> > On Jan 8, 2019, at 9:21 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > On Mon, 2019-01-07 at 19:58 -0500, Kim Barrett wrote:
> > > Please review this replacement of the "private" FreeIdSet with
> > > G1FreeIdSet, which has a new implementation using a semaphore
> > > rather
> > > than a monitor.
> > > 
> > > […]
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8216258
> > > 
> > > Webrev:
> > > http://cr.openjdk.java.net/~kbarrett/8216258/open.00/
> > > 
> > > Testing:
> > > mach5 tier1-5.
> > 
> >  thanks for the cleanup.
> 
> Thanks for looking at it.
> 
> > Some notes:
> > 
> > - is it possible to add a destructor that deletes the G1FreeIdSet
> > instance? I am aware that currently the destructor of
> > DirtyCardQueueSet
> > is never called, but still it would be nice to clean up afterwards.
> 
> I was going to say "but it's right there", but then realized what you
> were complaining about was the pre-existing lack of a delete of the
> freeidset by the non-existent ~DirtyCardQueueSet.

Yes, sorry, this was a pre-existing issue.

> > - not sure exactly why uintx is used for
> > G1FreeIdSet::_head_index_mask and _head: uintx are 32 bit on 32 bit
> > machines, so the update counter will be constrained by 32 bit -
> > bits(size) there anyway.
> > 
> > On 64 bit uintx is 64 bit, so the update counter does have the full
> > 32 bit range available, which is probably why uintx has been chosen
> > anyway?
> > 
> > I do think that even on 32 bits the remaining range should be
> > sufficient so it seems okay.
> 
> uintx is used as the type for _head (which affects others, like
> _head_index_mask) in order to maximize the size of the update
> counter. There's also a check that the size of that counter is not
> "too small", e.g. it must be at least BitsPerWord/2.
> 
> Because the size is a thread count (and currently derived from the
> number of processors), the assumption is that on a 32bit platform it
> will be relatively small compared to a 32bit uintx, and on a 64bit
> platform it will be relatively small compared to a 64bit uintx
> (though possibly "largish" compared to a 32bit uint).

I fully agree with that assessment. Just a comment.

> 
> > - maybe the cmpxchg loops in G1FreeIdSet::claim_par_id() and
> > release_par_id()  could be factored out into a helper somehow.
> 
> While the cmpxchg loops look structurally fairly similar (don't most
> CAS loops look more or less like this?), they really are pretty
> different in detail. I couldn't think of a way to produce a non-ugly,
> non-obfuscating, common helper.
> 

Okay, I know...

> > - I would prefer if the "claimed" constant were camel cased like
> > static consts should be to make it more distinguishable to regular
> > local variables.
> 
> Done.

Thanks.

> 
> > - pre-existing: I have no idea what the documentation of
> > G1FreeIdSet "// Represents a set of free small integer ids" is
> > supposed to mean. Could you fix that comment for the G1FreeIdSet
> > class please?
> 
> Changed it to:
> 
> // Represents a set of small integer ids, from which elements can be
> // temporarily allocated for exclusive use.  The ids are in a
> // contiguous range from 'start' to 'start + size'.  Used to obtain a
> // distinct worker_id value for a mutator thread that doesn't 
> // normally have such an id.

Perfect.

> 
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8216258/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8216258/open.01.inc/
> 
> 

Looks good.

Thomas