RFR(s): jcmd VM.classloaders should fold similar loaders

[David Holmes]

On 28/06/2018 2:09 AM, Thomas Stüfe wrote:
> Hi David,
> 
> since I have my two reviews, I would like to push this before the door
> closes tomorrow afternoon. Do you have any serious objections left?

No, my comments are just that. I have no vested interest in this code. 
The folding just seems somewhat subjective.

Cheers,
David

> Thanks, Thomas
> 
> On Wed, Jun 27, 2018 at 2:21 PM, David Holmes <david.holmes at oracle.com> wrote:
>> Hi Thomas,
>>
>>
>> On 27/06/2018 10:13 PM, Thomas Stüfe wrote:
>>>
>>> Hi David,
>>>
>>> On Mon, Jun 25, 2018 at 7:48 AM, David Holmes <david.holmes at oracle.com>
>>> wrote:
>>>>
>>>> Hi Thomas,
>>>>
>>>>
>>>> On 23/06/2018 5:10 AM, Thomas Stüfe wrote:
>>>>>
>>>>>
>>>>> Resent with the correct subject, sorry.
>>>>>
>>>>> ..Thomas
>>>>>
>>>>> On Fri, Jun 22, 2018 at 9:08 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> may I have reviews for this small enhancement to the jcmd
>>>>>> VM.classloader diagnostic command:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8205531
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~stuefe/webrevs/8205531-vm.classloader-tree-folding/webrev.00/webrev/
>>>>>>
>>>>>>
>>>>>> VM.classloaders prints a tree of class loaders. This tree can grow a
>>>>>> lot and become unwieldy, especially with a lot of similar loaders. One
>>>>>> prime example is the DelegatingClassLoader. It would be helpful were
>>>>>> all these loaders:
>>>>>>
>>>>>> 13114:
>>>>>> +-- <bootstrap>
>>>>>>          |
>>>>>>          +-- "platform",
>>>>>> jdk.internal.loader.ClassLoaders$PlatformClassLoader
>>>>>>                |
>>>>>>                +-- "app",
>>>>>> jdk.internal.loader.ClassLoaders$AppClassLoader
>>>>>>                      |
>>>>>>                      +-- test3.internals.InMemoryClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>>                            |
>>>>>>                            ...... repeat 1495 times
>>>>>>
>>>>>>     folded into one:
>>>>>>
>>>>>> 13114:
>>>>>> +-- <bootstrap>
>>>>>>          |
>>>>>>          +-- "platform",
>>>>>> jdk.internal.loader.ClassLoaders$PlatformClassLoader
>>>>>>                |
>>>>>>                +-- "app",
>>>>>> jdk.internal.loader.ClassLoaders$AppClassLoader
>>>>>>                      |
>>>>>>                      +-- test3.internals.InMemoryClassLoader
>>>>>>                            |
>>>>>>                            +--
>>>>>> jdk.internal.reflect.DelegatingClassLoader
>>>>>> (+ 1499 more)
>>>>
>>>>
>>>>
>>>> I don't quite understand that. These are different instances aren't they
>>>> -
>>>> potentially uniquely named? So if we gave all loaders a default name
>>>> (like
>>>> we do threads) they would all show up expanded again - right?
>>>>
>>>
>>> This patch will fold loaders loaded by the same parent, having the
>>> same class and the same or no name.
>>>
>>> Example:
>>>
>>> 25401:
>>> +-- <bootstrap>
>>>         |
>>>         +-- "platform",
>>> jdk.internal.loader.ClassLoaders$PlatformClassLoader
>>>         |     |
>>>         |     +-- "app", jdk.internal.loader.ClassLoaders$AppClassLoader
>>>         |
>>>         +-- "small-loader", test3.internals.InMemoryClassLoader (+ 1101
>>> more)
>>>
>>> Note that I compare addresses of _Symbol_. So, loaders which have the
>>> _same_ default name still fold, see example above.
>>
>>
>> Right. So in the case of DelegatingClassloader, today this change will fold
>> them all into one because they are unnamed. If tomorrow we give them a
>> unique name then they will no longer fold. There's no substantive difference
>> between the two cases that I can see so why should they present differently?
>> I guess I'm not sure the name is actually significant here.
>>
>> What happens when some of these similar loaders have distinct child loaders?
>> How will things be grouped and displayed then?
>>
>> Thanks,
>> David
>>
>>
>>> Thanks, Thomas
>>>
>>>
>>>> typo:
>>>>
>>>> !   // we we add a
>>>>
>>>> Not a review as I don't know any of the logic being modified.
>>>>
>>>> David
>>>>
>>>>
>>>>>>
>>>>>>
>>>>>> Original idea by Bernd Eckenfels, see
>>>>>>
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-May/023824.html
>>>>>>
>>>>>> Thank you, Thomas