RFR 8190359: Reduce the number of recorded klass dependencies

Thu Feb 1 14:55:42 UTC 2018

Thanks!

Harold

On 2/1/2018 9:39 AM, Lois Foltan wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>