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

[Stefan Johansson]

Thanks for the reviews Stefan, Thomas and Mikael,

StefanJ

On 2016-10-05 11:55, Stefan Karlsson wrote:
> 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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>