RFR 8190359: Reduce the number of recorded klass dependencies

[harold seigel]

Hi,

Please review this updated webrev containing the changes suggested by Ioi.

http://cr.openjdk.java.net/~hseigel/bug_8190359.2/webrev/index.html

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