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

[Jon Masamitsu]

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.

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