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

[Stefan Karlsson]

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

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