Review request: 8013160: NPG: Remove unnecessary mark stack draining after CodeCache::do_unloading

[Jon Masamitsu]

http://cr.openjdk.java.net/~stefank/8013160/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.frames.html

I understand that the point of the change was that CodeCache::do_unloading()
cannot not add to the marking stack anymore but the
original code was less trusting in that it called for verification
when it really thought it was not needed.  For example,

> 6047   verify_work_stacks_empty();

Any second thoughts on removing the later calls to 
verify_work_stacks_empty()?
If you think it can never happen and it's not worth checking (and cluttering
the code) no problem.  Either way, looks good.

Jon



On 4/26/2013 1:15 AM, Stefan Karlsson wrote:
> On 04/25/2013 11:47 PM, Coleen Phillimore wrote:
>>
>> Hi, Sorry Stefan I think I liked your .00 version better.   Now 
>> there's a level of indirection and it doesn't seem to save any code 
>> because the call sites are slightly different.
>
> It only saves code duplication in four of the collectors, the other 
> two still does the unloading differently.
>
> Could someone else review the .00 version?
> http://cr.openjdk.java.net/~stefank/8013160/webrev.00/
>
> thanks,
> StefanK
>
>>
>> Coleen
>>
>> On 04/25/2013 08:07 AM, Stefan Karlsson wrote:
>>> On 04/24/2013 11:42 PM, Coleen Phillimore wrote:
>>>> On 4/24/2013 4:49 PM, Stefan Karlsson wrote:
>>>>> On 4/24/13 10:32 PM, Coleen Phillimore wrote:
>>>>>>
>>>>>> Stefan,
>>>>>>
>>>>>> Is there a common place for this code?
>>>>>
>>>>> I don't think there's any common place in the GCs for this. I'm 
>>>>> open for suggestions if you know a good place in the runtime code.
>>>>
>>>> memory/genCollectedHeap?   I have no idea...
>>>>>
>>>>>> It's 4 copies of the similar code, which takes me a while to find 
>>>>>> when I set out looking for it.
>>>>>
>>>>> Yes, I tried to make them look alike so that we/I have an 
>>>>> opportunity to extract it out to one function. The fact that CMS 
>>>>> does this differently made me hesitant to combine them.
>>>>>
>>>>>> In the future, I think StringTable::unlink() could be moved after 
>>>>>> CLDG::purge() which I thought should be around here too (but it's 
>>>>>> not).   The only reason StringTable::unlink() used to be here was 
>>>>>> because it used to have Oops in it.
>>>>>
>>>>> Don't you mean the SymbolTable? I'm OK with moving the 
>>>>> SymbolTable, but not the StringTable ...
>>>>
>>>> I did mean SymbolTable.
>>>>
>>>>>
>>>>>>    Can you unite this duplicated code?
>>>>>
>>>>> It's easy to unit the code for psMarkSweep, genMarkSweep, 
>>>>> g1MarkSweep and psParallelCompact but it's a bit more work to also 
>>>>> unify the CMS code. Would it be good enough for this patch if I 
>>>>> united the former GCs? And maybe handle CMS when G1 also starts 
>>>>> doing class unloading?
>>>>
>>>> Ok, keep it in mind for future work.  It would be nice to have the 
>>>> runtime GC things be called in one place or easy to find.
>>>>
>>>> I semi-reviewed the code and except for the GC part (which is most 
>>>> of the change).   We don't pass the keep_alive closure anymore for 
>>>> compiledICHolders so I guess this makes sense if this is what the 
>>>> keep_alive closure did.   So this is a partial review.
>>>
>>> I've update the patch to gather the unloading code in a helper class 
>>> called GCUnloading. What do you think?
>>>
>>> http://cr.openjdk.java.net/~stefank/8013160/webrev.01/
>>>
>>> thanks,
>>> StefanK
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>>
>>>>> thanks,
>>>>> StefanK
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 4/24/2013 4:18 PM, Stefan Karlsson wrote:
>>>>>>> http://cr.openjdk.java.net/~stefank/8013160/webrev.00/
>>>>>>>
>>>>>>> There's some remnants from the PermGen that's confusing in the 
>>>>>>> class unloading code. This patch removes the unnecessary code, 
>>>>>>> cleans up some comments and unifies the code structure over 
>>>>>>> different GCs.
>>>>>>>
>>>>>>> From the Bug report:
>>>>>>> ---
>>>>>>> The PermGen Removal changed CompiledICHolders from being Java 
>>>>>>> objects to C++ objects.
>>>>>>>
>>>>>>> CodeCache::do_unloading(...) used to take a keep_alive closure 
>>>>>>> that was applied to CompiledICHolders deep down in 
>>>>>>> nmethod::do_unloading. Since CompileICHolders don't move 
>>>>>>> anymore, and we have other ways to keep them alive, we don't use 
>>>>>>> the keep_alive closures in these functions anymore.
>>>>>>>
>>>>>>> Because of this, CodeCache::do_unloading will not populate the 
>>>>>>> marking stacks, and we can remove the call to drain the stacks 
>>>>>>> after the calls to CodeCache::do_unloading.
>>>>>>>
>>>>>>> This also has the neat effect that we now can verify that the 
>>>>>>> marking has really completed before we start unloading classes, 
>>>>>>> which makes the code easier to maintain/understand.
>>>>>>> ---
>>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>
>>>>>
>>>>
>>>
>>
>
>