RFR: 8216258: Make FreeIdSet semaphore-based

[Thomas Schatzl]

Hi,

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.
> 
> The representation of the set is largely the same, being an array of
> "next" indices into the array, with a "head" value containing the
> current first index.  However, the manipulation of the set is now
> done
> without a lock, using CAS to change the head.  To avoid ABA, the head
> value also contains an update counter which is incremented on each
> modification of the head.
> 
> Also generalized to allow the id set to start with a non-zero id,
> because that looks like it might simplify other changes I'm working
> on. 
> 
> The entire "free id set" has been moved to new files and names (with
> the usual G1 prefixing), to permit gtest-based unit testing.
> 
> 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.

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.

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

- maybe the cmpxchg loops in G1FreeIdSet::claim_par_id() and
release_par_id()  could be factored out into a helper somehow.

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

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

Thanks,
  Thomas