RFR 8190359: Reduce the number of recorded klass dependencies

[Lois Foltan]

Looks good!
Lois

On 2/1/2018 9:14 AM, harold seigel wrote:
> Hi,
>
> Please review version 3 of this change:
>
> http://cr.openjdk.java.net/~hseigel/bug_8190359.3/webrev/index.html
>
> The only change from the previous version is the addition of lines 
> 342-347 to classLoaderData.cpp to handle the case pointed out by Coleen.
>
> Thanks, Harold
>
>
> On 1/31/2018 4:07 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>