RFR 8190359: Reduce the number of recorded klass dependencies

harold seigel harold.seigel at oracle.com
Wed Jan 31 20:18:11 UTC 2018


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.

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



More information about the hotspot-runtime-dev mailing list