8165949: Serial and ConcMarkSweep do not unload strings when class unloading is disabled

[Stefan Karlsson]

Hi StefanJ,

On 2016-09-21 15:30, Stefan Johansson wrote:
> ThanksMikael,
>
> Got some offline comments from Stefan K and updated the change with
> regard to those comments:
> Full: http://cr.openjdk.java.net/~sjohanss/8165949/hotspot.02/
> Inc: http://cr.openjdk.java.net/~sjohanss/8165949/hotspot.01-02/

This looks good and unifies the Full GC implementations, so consider 
this patch reviewed.

Note that the concurrent marking cycles of both CMS and G1 don't clean 
the StringTable when ClassUnloadingWithConcurrentMark is disabled. This 
is different to how the STW collectors treat the StringTable when 
ClassUnloading is disabled.

Thanks,
StefanK

>
> The main thing that has changed is that the string table processing has
> moved out to its own function, similar to what was done in G1 root
> processing a few weeks back. This allows us to keep setting up the
> weak_roots_closure the same way as before, and thus make use of an
> optimization in SystemDictionary processing which was lost in my
> previous patch.
>
> Thanks,
> Stefan
>
> On 2016-09-20 15:12, Mikael Gerdin wrote:
>> Hi Stefan
>>
>> On 2016-09-20 14:55, Stefan Johansson wrote:
>>> Hi Mikael,
>>>
>>> On 2016-09-20 14:24, Mikael Gerdin wrote:
>>>> Hi,
>>>>
>>>> On 2016-09-20 11:34, Stefan Johansson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this fix for:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8165949
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~sjohanss/8165949/hotspot.00/
>>>>
>>>> As mentioned in my other review the comment above enum ScanningOption
>>>> might need an update.
>>>>
>>>> I think I'd prefer cms_process_roots instead of conc_process_roots
>>>> because I think it should be clear that this is cms-specific code.
>>>>
>>> Thanks for reviewing, change the name per your request:
>>> Full: http://cr.openjdk.java.net/~sjohanss/8165949/hotspot.01/
>>> Inc: http://cr.openjdk.java.net/~sjohanss/8165949/hotspot.00-01/
>>
>> Looks good.
>> /Mikael
>>
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> Otherwise it looks good!
>>>> /Mikael
>>>>
>>>>
>>>>>
>>>>> Summary:
>>>>> This fix builds on top of the sub-task cleanup out for review in:
>>>>> 8166276: Refactor gen_process_roots to allow simpler fix for 8165949
>>>>>
>>>>> The cleanup task splits gen_process_roots into two version, one for
>>>>> young and one for old. This fix takes this one step further and
>>>>> split up
>>>>> the old version to have one version for the full-GC case and one
>>>>> for the
>>>>> concurrent case used in CMS. This simplifies the methods somewhat and
>>>>> also allows to do the bug fix very simple.
>>>>>
>>>>> The problem we want to solve is for full-GCs when ClassUnloading is
>>>>> disabled. In this case the old code translated ClassUnloading =
>>>>> false to
>>>>> mark all oops as live, but this is not true for strings in the string
>>>>> table. So after the split of old_process_roots into conc_process_roots
>>>>> and full_process_roots, full_process_roots can use the is_adjust_phase
>>>>> flag to avoid treating all oops in the marking phase.
>>>>>
>>>>> Testing:
>>>>> * JTREG tests locally
>>>>> * RBT on hs-gc tier 2-4
>>>>> * Verified that test/runtime/interned/SanityTest.java works for all GC
>>>>> with ClassUnloading disabled after the fix.
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>>
>>>>>
>>>
>