RFR 8190359: Reduce the number of recorded klass dependencies

Thu Feb 1 14:14:11 UTC 2018

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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>