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

[Stefan Karlsson]

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/f05bb4f1/attachment.htm>