Review request: 8013160: NPG: Remove unnecessary mark stack draining after CodeCache::do_unloading
Stefan Karlsson
stefan.karlsson at oracle.com
Mon Apr 29 06:57:16 UTC 2013
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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
More information about the hotspot-gc-dev
mailing list