RFR 8190359: Reduce the number of recorded klass dependencies

[coleen.phillimore at oracle.com]

On 1/31/18 3:18 PM, harold seigel wrote:
> Hi Lois,
>
> Thanks for looking at this.
>
> Please see comments in-line.
>
>
> On 1/31/2018 2:59 PM, Lois Foltan wrote:
>> On 1/31/2018 2:36 PM, harold seigel wrote:
>>
>>> Hi,
>>>
>>> Please review this updated webrev containing the changes suggested 
>>> by Ioi.
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8190359.2/webrev/index.html
>>
>> Looks okay.  I have a couple of minor comments:
>>
>> 1. Why you were able to remove the check at line #347?  It seems that 
>> now, if 'from's CLD is anonymous and also is in a parent CLD of 'to', 
>> a dependency will no longer be added for it, where in the old code it 
>> was?
> if from's CLD is anonymous and 'from' is a parent CL of 'to' the 
> dependency will still get recorded because 
> java_lang_ClassLoader::isAncestor(from, to) returns TRUE if 'to' is an 
> ancestor of 'from', but FALSE if 'from' is an ancestor of 'to'.  So, 
> in this case, it will return FALSE.

So this is very confusing!

I think I get it.    If "from" is anonymous and "to" is not anonymous 
(already checked above), "to" won't get unloaded when it's an ancestor 
of "from"s class_loader.

There is the missing case if both are anonymous, you should check for 
equality at line 343.

Otherwise, this looks really good.   I like that you're using isAncestor.
>
>> 2. Just very minor, I don't prefer the new method 
>> is_permanent_class_loader_data, I would have preferred fixing 
>> is_builtin_class_loader to handle correctly the anonymous class 
>> loader data, but my understanding is that there is a later RFE that 
>> will address this, correct?
> Yes. See JDK-8190235 <https://bugs.openjdk.java.net/browse/JDK-8190235>.

The name "permanent" is odd but I can't think of a more concise name 
that means _is_not_unloaded.

Thanks,
Coleen
>
> Thanks! Harold
>>
>> Thanks,
>> Lois
>>
>>
>>>
>>> Thanks! Harold
>>>
>>>
>>> On 1/30/2018 7:07 PM, Ioi Lam wrote:
>>>>
>>>>
>>>> On 1/30/18 12:30 PM, harold seigel wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> Thanks for looking at this!  Please see comments inline.
>>>>>
>>>>> On 1/30/2018 2:50 PM, Ioi Lam wrote:
>>>>>> Hi Harold,
>>>>>>
>>>>>> Why is this needed
>>>>>>
>>>>>>  336   if (from_cld == to_cld ...
>>>>> It's not needed. I added it as a quick way to exclude many 
>>>>> potential dependencies.  I can remove it.
>>>>>>
>>>>>> It seems like it's already checked by here
>>>>>>
>>>>>>  350     if (from == to || 
>>>>>> java_lang_ClassLoader::isAncestor(from, to)) {
>>>>> You are right. It is.  The first 'if' clause also handles the case 
>>>>> where 'from_cld' and 'to_cld' have the same class loader, but 
>>>>> differ because 'from_cld' is anonymous.
>>>>>>
>>>>>> It's probably explained by this
>>>>>>
>>>>>>  348     assert(from != to || from_cld->is_anonymous(), "sanity 
>>>>>> check");
>>>>> If line 336 is removed then I'll remove this assert because it's 
>>>>> no longer valid.
>>>>>>
>>>>>> ... but I think this is a rather tricky area so either (a) more 
>>>>>> comments might be needed in the source code, or (b) simplify the 
>>>>>> code so it requires less explanation.
>>>>> Does removing lines 336 and 348 simplify the code?
>>>>
>>>> Hi Harold，
>>>>
>>>> Thanks for the explanation. I think removing these lines will make 
>>>> the code much easier to understand.
>>>>
>>>>>>
>>>>>> Also, I wonder if it's possible to add a new test case for this.
>>>>> Any suggestions on how to do this?  If logging was added then it 
>>>>> would be easier to write tests, but I don't think logging is 
>>>>> needed here because recording dependencies is not something users 
>>>>> care about.  Recording dependencies is an implementation detail.
>>>>>
>>>> Ideally we should have a negative test case that tries to violates 
>>>> loader constraints, but is prevented to do so even when the 
>>>> constraints aren't recorded in this case. However, I've no idea how 
>>>> to write that :-(
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>> Thanks, Harold
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 1/30/18 10:38 AM, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this RFR for JDK-11 to reduce the number of class 
>>>>>>> dependencies recorded by the VM.  The change primarily does this 
>>>>>>> by not recording dependencies to classes that are loaded by a 
>>>>>>> builtin class loader and are not anonymous. These classes never 
>>>>>>> get unloaded, so no recorded dependency is needed.
>>>>>>>
>>>>>>> Additionally, the change simplifies the code that deals with 
>>>>>>> when the classes have the same class loader and when the 
>>>>>>> dependency is to a class loaded by a parent loader.
>>>>>>>
>>>>>>> Open Webrev: 
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8190359/webrev/index.html
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8190359
>>>>>>>
>>>>>>> The change was tested with JPRT, Mach 5 tier1 - tier5 tests, and 
>>>>>>> non-colocated tonga tests.  Additionally, print statements were 
>>>>>>> temporarily added to the code and the output analyzed to check 
>>>>>>> that dependencies were being correctly recorded or not recorded.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>
>>>>>
>>>>
>>>
>>
>