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

[Stefan Karlsson]

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.

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