Request for code review - JDK-8141123 Change int's to size_t in FreeIdSet

[Kim Barrett]

On Nov 26, 2015, at 6:13 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Wed, 2015-11-25 at 17:34 -0500, Kim Barrett wrote:
>> On Nov 12, 2015, at 4:35 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>> 
>>> - FreeIdSet::set_safepoint() seems to be never called. I think it
>>> allows you to remove a *lot* of unused stuff.
>> 
>> Perhaps the bug here is that there should be calls to set_safepoint but they are missing?
>> 
> 
>  apparently the functionality is not needed, that is, it has not even
> been in use in the initial integration of G1 (and it has been added in
> that change).
> 
> So I suggest to just remove it and everything that depended on it
> instead of trying to find out what this functionality may have been used
> for.

Or this could be a "been there forever" latency bug. 

Consider a situation where concurrent refinement is significantly into
the red zone, e.g. lots of mutator threads have decided they need to
refine their own DCQ because concurrent refinement is significantly
behind.  Each of those mutator threads attempts to allocate a
pseudo-worker_id from the FreeIdSet.  If there are many such threads,
some of them will block, waiting for a free id.  Now we attempt to
enter a safepoint.  Not only do we now wait for all the mutator
threads that are actively processing buffers to complete and move on
to a safepoint check somewhere, but we also need to wait for the
threads that were waiting for a free id.  The set_safepoint function
pretty clearly exists to cancel the latter delay.  However, I wonder
if a better approach might not be to change the mutex used for that
free id allocation to be a safpoint checking mutex.  [I think that
would require an additional mutex for DCQS to protect its FreeIdSet,
since presently the same mutex is used to protect the completed buffer
list, and that gets called from GC worker threads, so can't check for
safepoint.]

However, given that we've apparently never had any callers of
set_safepoint, it is perhaps OK to go ahead and remove it, and have
additional followups to look into the various issues mentioned here.

Note: This whole FreeIdSet thing seems like a whole lot of complex
mechanism for what it accomplishes.  There is presently one user of
this facility, DirtyCardQueueSet::mut_process_buffer.  It is used
there to associate a pseudo-worker_id with a mutator (Java) thread
that is going to process a dirty card buffer.  That pseudo-worker_id
is passed down through several layers of API until it reaches
FromCardCache::contains_or_replace, where it is used as an index into
a per-worker cache array.  The FromCardCache has to be sized to
account for both the actual worker threads and the (one and only)
FreeIdSet associated with mut_process_buffer.  

For further followup, I think JavaThread::_claimed_par_id and the
associated getter/setter are not needed.  The only place they are used
is where mut_process_buffer checks for the thread already having an id
and only allocating one if it doesn't.  But that's the only place
where one gets allocated and assigned, and its always released on the
way out.  And mut_process_buffer never recursively calls itself.  As a
result, the check for an already assigned worker_id will never
succeed.  So the whole _claimed_par_id mechanism appears to be a waste
of time and space.  And while the thread field is named similarly to
the FreeIdSet accessor (claim_par_id), there cannot be multiple
FreeIdSets associated with the thread field.

If I recall correctly, DirtyCardQueueSet is a singleton, and could be
an AllStatic with a bit of work.  Given that it is the only client of
FreeIdSet, the latter's support for multiple sets is also unnecessary.
And given there are no other clients of FreeIdSet, maybe that class
should be an internal detail of DirtyCardQueueSet.