RFR: 8216258: Make FreeIdSet semaphore-based

[Kim Barrett]

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

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

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

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

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

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