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

[Thomas Schatzl]

Hi,

On Tue, 2015-12-01 at 18:45 -0500, Kim Barrett wrote:
> 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.
> 

  ... and reading through all this investigation, actually I think the
whole FreeIdSet is unnecessary. The mutator thread could just take any
random valid thread id and the FromCardCache would still work (although
at reduced efficiency, but that is the case in that situation already
anyway).

Ideally there would be some small extra cache, or we just do not use the
cache in this situation which seems a reasonable option too (or let all
mutators use a single equal extra id), to not thrash the FromCardCache
of the refinement threads.

I am leaving it to Alex and you to decide whether you want this cleanup
as an intermediate step, or go for a full cleanup.

Thanks,
  Thomas