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

[Coleen Phillimore]

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.

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