RFR 8190359: Reduce the number of recorded klass dependencies

[harold seigel]

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

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