RFR 8190359: Reduce the number of recorded klass dependencies

[Lois Foltan]

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

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