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

[Jon Masamitsu]

On 4/29/13 7:56 AM, Jon Masamitsu wrote:
>
> On 4/28/13 11:57 PM, Stefan Karlsson wrote:
>> On 04/26/2013 07:09 PM, Jon Masamitsu wrote:
>>> http://cr.openjdk.java.net/~stefank/8013160/webrev.00/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.frames.html 
>>>
>>>
>>> I understand that the point of the change was that 
>>> CodeCache::do_unloading()
>>> cannot not add to the marking stack anymore but the
>>> original code was less trusting in that it called for verification
>>> when it really thought it was not needed.  For example,
>>>
>>>> 6047   verify_work_stacks_empty();
>>>
>>> Any second thoughts on removing the later calls to 
>>> verify_work_stacks_empty()?
>>> If you think it can never happen and it's not worth checking (and 
>>> cluttering
>>> the code) no problem.  Either way, looks good.
>>
>> I wanted to remove the later, unnecessary calls to 
>> verify_work_stacks_empty(), to clearly mark the place above where we 
>> stop populating the marking stacks.
>>
>
> Fair enough.

And the rest looks good.

Jon

>
> Jon
>
>> thanks,
>> StefanK
>>
>>>
>>> Jon
>>>
>>>
>>>
>>> On 4/26/2013 1:15 AM, 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/
>>>>
>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>