RFR: 8216258: Make FreeIdSet semaphore-based

[sangheon.kim at oracle.com]

Hi Kim,

On 1/8/19 5:25 PM, 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.
>
>> - 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/
Webrev.01 looks very nice.
Sorry for the late review.

Thanks,
Sangheon

>
>