RFR 8190359: Reduce the number of recorded klass dependencies

[coleen.phillimore at oracle.com]

On 1/31/18 4:02 PM, harold seigel wrote:
> Hi Coleen,
>
> Thanks for reviewing this.
>
> See comments in-line.
>
>
> On 1/31/2018 3:49 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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.
> Yes.
>>
>> There is the missing case if both are anonymous, you should check for 
>> equality at line 343.
> I think that dependency has to be recorded.  If both are anonymous 
> then by definition they have different CLD's.  And, even if they have 
> the same CL, or to's CL is the parent of from's, the dependency has to 
> be recorded because, being anonymous, 'to' can go away at any time.

Oh, but if toCLD == fromCLD, not their class loaders.  ie, if you're 
recording a dependency to itself in the anonymous case.

>>
>> 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.
> I thought of 'eternal' or 'immortal' but prefer 'permanent'.
>

ha ha!
Coleen

> Thanks, Harold
>>
>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>