RFR(S): 8005032: G1: Cleanup serial reference processing closures in concurrent marking

[Bengt Rutisson]

Hi John,

Looks good! Thanks for fixing this!

A couple of comments:

In ConcurrentMark::weakRefsWork() there is this code:

2422     if (rp->processing_is_mt()) {
2423       // Set the degree of MT here.  If the discovery is done MT, there
2424       // may have been a different number of threads doing the 
discovery
2425       // and a different number of discovered lists may have Ref 
objects.
2426       // That is OK as long as the Reference lists are balanced (see
2427       // balance_all_queues() and balance_queues()).
2428       rp->set_active_mt_degree(active_workers);
2429     }

Could we now always call rp->set_active_mt_degree() ? Maybe I am missing 
the point here, but I thought that we are now using queues and all 
rp->set_active_mt_degree() does is set the number of queues to 
active_workers. Which will be 1 for the single threaded mode.

If we do that we can also remove the first part of the assert a bit 
further down:

2449     assert(!rp->processing_is_mt() || rp->num_q() == 
active_workers, "why not");


But I didn't have time to follow this code properly, so maybe I'm way 
off here…


Also, I think I would like to move the code you added to 
G1CMParDrainMarkingStackClosure::do_void() into 
ConcurrentMark::weakRefsWork() somewhere. Maybe something like:

if (!rp->processing_is_mt()) {
   set_phase(1, false /* concurrent */);
}

It is a bit strange to me that G1CMParDrainMarkingStackClosure should 
set this up. If we really want to keep it in 
G1CMParDrainMarkingStackClosure I think the constructor would be a 
better place to do it than do_void().



Some minor comments:

In G1CMParKeepAliveAndDrainClosure and G1CMParDrainMarkingStackClosure 
constructors there is this assert:

assert(_task->worker_id() == 0 || _is_par, "sanity");

I think it is good, but I had to think a bit about what it meant and to 
me I think it would be quicker to understand if it was the other way around:

assert(_is_par || _task->worker_id() == 0, "Only worker 0 should be used 
if single threaded");

But maybe it is just me...



Remove newline on line 2254 ?


ConcurrentMark::weakRefsWork()

How about introducing a variable that either hold the value of 
&par_task_executor or NULL depending on rp->processing_is_mt()? That way 
we don't have to duplicate and inline this test twice:

(rp->processing_is_mt() ? &par_task_executor : NULL)



As a separate change it might be worth renaming the closures to not have 
"Par" in the name, now that they are not always parallel...

G1CMParKeepAliveAndDrainClosure -> G1CMKeepAliveAndDrainClosure
G1CMParDrainMarkingStackClosure -> G1CMDrainMarkingStackClosure

But it would be very confusing to do this in the same change.

Thanks,
Bengt


On 1/8/13 7:59 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> Can I have a couple of volunteers review the changes for this CR - the 
> webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/8005032/webrev.0/
>
> Summary:
> The previous serial keep-alive and complete-gc reference processing 
> oop closures used during concurrent marking operated directly on the 
> global marking stack while the parallel closures used the local task 
> queues from the concurrent marking task objects (using the global 
> marking stack as a backing store). Additionally the parallel 
> keep-alive closure also drained the local task queue after processing 
> a given number of references.
>
> These changes make the serial reference processing code use the same 
> oop closures as the parallel code. This will reduce the likelihood of 
> hitting a marking stack overflow while processing the discovered 
> references during the remark phase.
>
> Testing:
> GC test suite with a low IHOP and marking verification.
> Test case for 8004812 (Kitchensink) with  a very low marking stack 
> size (4K entries) and heap verification, and both with and without 
> ParallelRefProcEnabled and ParallelGCThreads=0.
> jprt.
>
> Thanks,
>
> JohnC