RFR: 8216258: Make FreeIdSet semaphore-based
thomas.schatzl at oracle.com
Tue Jan 8 14:21:51 UTC 2019
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
> 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
> The entire "free id set" has been moved to new files and names (with
> the usual G1 prefixing), to permit gtest-based unit testing.
> mach5 tier1-5.
thanks for the cleanup.
- 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
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
- 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?
More information about the hotspot-gc-dev