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

[Coleen Phillimore]

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.

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
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130425/d80622bb/attachment.htm>