RFR 8190359: Reduce the number of recorded klass dependencies

[harold seigel]

Thanks Lois!

Harold

On 1/31/2018 3:56 PM, Lois Foltan wrote:
>
> On 1/31/2018 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.
>
> I think I phrased my question wrong, it's the case where from is an 
> anonymous CLD and 'to' is an ancestor of 'from'.  There is a change in 
> behavior here.  The old code used to always record a dependency if 
> 'from' is an anonymous cld.  The new code does not record that 
> dependency, in the case where 'to' is an ancestor of 'from', as you 
> point out above, isAncestor(from, to) will return TRUE and the 
> dependency will not be recorded.   However, thanks for explaining in a 
> side email that in this scenario, 'to' will never go away before 
> 'from', so I agree the change is behavior is seems ok.
>
> Thanks,
> Lois
>
>>
>>> 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>.
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>