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

[Mikael Gerdin]

Stefan,

On 2013-04-26 10:15, 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/

It looks good to me.

/Mikael


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