RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore
Summary: added dcmd for printing system dictionary like the stringtable
and symboltable and making print functions go to outputstream rather
than tty

Tested with tier1 on linux x64, and runThese with jcmd to query
systemdictionaries (lots of class loaders).

open webrev at http://cr.openjdk.java.net/~coleenp/8184994.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8184994

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

Aleksey Shipilev-4
On 08/01/2017 04:36 PM, [hidden email] wrote:
> Summary: added dcmd for printing system dictionary like the stringtable and symboltable and making
> print functions go to outputstream rather than tty
>
> Tested with tier1 on linux x64, and runThese with jcmd to query systemdictionaries (lots of class
> loaders).
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8184994

Cursory review:

*) Not entirely clear why this include in compactHashtable.cpp:

  32 #include "runtime/vmThread.hpp"

*) I guess these pairs of lines may be coalesced in dictionary.cpp:

 444   st->print_cr("^ indicates that initiating loader is different from "
 445                "defining loader");

 454       st->print("%4d: %s%s", index, is_defining_class ? " " : "^", e->external_name());
 455       st->print(", loader ");


*) dictionary.hpp: stray whitespace at the end

 106   void print_on(outputStream* st) const ;

*) placeholders.cpp: while we are here, probably worth changing print("\n") to cr()?

*) placeholders.cpp: coalesce?

 232       st->print("%4d: ", pindex);
 233       st->print("placeholder ");

*) placeholders.hpp: method name is camel-cased, can rename it?

 130   void printActionQ(outputStream* st) {

*) systemDictionary.cpp: in SystemDictionary::dump, the if(verbose) condition seems inverted. Should
dump tables when verbose?


Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore


On 8/1/17 10:53 AM, Aleksey Shipilev wrote:

> On 08/01/2017 04:36 PM, [hidden email] wrote:
>> Summary: added dcmd for printing system dictionary like the stringtable and symboltable and making
>> print functions go to outputstream rather than tty
>>
>> Tested with tier1 on linux x64, and runThese with jcmd to query systemdictionaries (lots of class
>> loaders).
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8184994
> Cursory review:
>
> *) Not entirely clear why this include in compactHashtable.cpp:
>
>    32 #include "runtime/vmThread.hpp"

There's a VMThread::vm_thread() call in cmopactHashtable.cpp.  I took
out #include diagnosticCommand.hpp from compactHashtable.hpp which
transitively included it.

I should have mentioned that a lot of this change was to include files
not transitively included by removing the dcmds from compactHashtable.hpp.
>
> *) I guess these pairs of lines may be coalesced in dictionary.cpp:
>
>   444   st->print_cr("^ indicates that initiating loader is different from "
>   445                "defining loader");
>
>   454       st->print("%4d: %s%s", index, is_defining_class ? " " : "^", e->external_name());
>   455       st->print(", loader ");
>
yes. that makes sense.

> *) dictionary.hpp: stray whitespace at the end
>
>   106   void print_on(outputStream* st) const ;

Fixed.

> *) placeholders.cpp: while we are here, probably worth changing print("\n") to cr()?

Yes, I should have looked at this more carefully to fix these and
coalesce these lines below.
>
> *) placeholders.cpp: coalesce?
>
>   232       st->print("%4d: ", pindex);
>   233       st->print("placeholder ");

fixed.
>
> *) placeholders.hpp: method name is camel-cased, can rename it?
>
>   130   void printActionQ(outputStream* st) {

Sure.   I'll rename it print_action_queue().
>
> *) systemDictionary.cpp: in SystemDictionary::dump, the if(verbose) condition seems inverted. Should
> dump tables when verbose?

I kept the name dump_table from the VM.stringtable and VM.symboltable
dcmd code, when the function really prints hashtable statistics.   I
could rename that function to be print_table_statistics() instead.  And
change ClassLoaderDataGraph::dump_dictionary to
print_dictionary_statistics.  If that makes sense.

Thanks,
Coleen

>
>
> Thanks,
> -Aleksey
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

Aleksey Shipilev-4
On 08/01/2017 07:14 PM, [hidden email] wrote:
>> *) systemDictionary.cpp: in SystemDictionary::dump, the if(verbose) condition seems inverted. Should
>> dump tables when verbose?
>
> I kept the name dump_table from the VM.stringtable and VM.symboltable dcmd code, when the function
> really prints hashtable statistics.   I could rename that function to be print_table_statistics()
> instead.  And change ClassLoaderDataGraph::dump_dictionary to print_dictionary_statistics.  If that
> makes sense.

Yeah, that would make sense.

Thanks,
-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore


On 8/1/17 1:19 PM, Aleksey Shipilev wrote:
> On 08/01/2017 07:14 PM, [hidden email] wrote:
>>> *) systemDictionary.cpp: in SystemDictionary::dump, the if(verbose) condition seems inverted. Should
>>> dump tables when verbose?
>> I kept the name dump_table from the VM.stringtable and VM.symboltable dcmd code, when the function
>> really prints hashtable statistics.   I could rename that function to be print_table_statistics()
>> instead.  And change ClassLoaderDataGraph::dump_dictionary to print_dictionary_statistics.  If that
>> makes sense.
> Yeah, that would make sense.

Thanks!  Here's a webrev with the renaming:

open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev

and tested.

Thanks,
Coleen

>
> Thanks,
> -Aleksey
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

Aleksey Shipilev-4
On 08/01/2017 07:32 PM, [hidden email] wrote:
> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev

Looks good to me, except for print("\n")-s in PlaceholderEntry::print_entry.

-Aleksey

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore


On 8/1/17 1:44 PM, Aleksey Shipilev wrote:
> On 08/01/2017 07:32 PM, [hidden email] wrote:
>> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev
> Looks good to me, except for print("\n")-s in PlaceholderEntry::print_entry.

Really fixed it now.  Thanks!
Coleen

>
> -Aleksey
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

harold seigel
In reply to this post by coleen.phillimore
Hi Coleen,

Other than this erroneous comment in diagnosticCommand.hpp, the change
looks good:

    735 // VM.systemdictionary -verbose: for dumping the*string table*

Go ahead and push it.

Thanks, Harold

On 8/1/2017 1:32 PM, [hidden email] wrote:

>
>
> On 8/1/17 1:19 PM, Aleksey Shipilev wrote:
>> On 08/01/2017 07:14 PM, [hidden email] wrote:
>>>> *) systemDictionary.cpp: in SystemDictionary::dump, the if(verbose)
>>>> condition seems inverted. Should
>>>> dump tables when verbose?
>>> I kept the name dump_table from the VM.stringtable and
>>> VM.symboltable dcmd code, when the function
>>> really prints hashtable statistics.   I could rename that function
>>> to be print_table_statistics()
>>> instead.  And change ClassLoaderDataGraph::dump_dictionary to
>>> print_dictionary_statistics.  If that
>>> makes sense.
>> Yeah, that would make sense.
>
> Thanks!  Here's a webrev with the renaming:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev
>
> and tested.
>
> Thanks,
> Coleen
>
>>
>> Thanks,
>> -Aleksey
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore

Thanks, Harold!   I fixed the comment.
Coleen

On 8/2/17 9:55 AM, harold seigel wrote:

> Hi Coleen,
>
> Other than this erroneous comment in diagnosticCommand.hpp, the change
> looks good:
>
>    735 // VM.systemdictionary -verbose: for dumping the*string table*
>
> Go ahead and push it.
>
> Thanks, Harold
>
> On 8/1/2017 1:32 PM, [hidden email] wrote:
>>
>>
>> On 8/1/17 1:19 PM, Aleksey Shipilev wrote:
>>> On 08/01/2017 07:14 PM, [hidden email] wrote:
>>>>> *) systemDictionary.cpp: in SystemDictionary::dump, the
>>>>> if(verbose) condition seems inverted. Should
>>>>> dump tables when verbose?
>>>> I kept the name dump_table from the VM.stringtable and
>>>> VM.symboltable dcmd code, when the function
>>>> really prints hashtable statistics.   I could rename that function
>>>> to be print_table_statistics()
>>>> instead.  And change ClassLoaderDataGraph::dump_dictionary to
>>>> print_dictionary_statistics.  If that
>>>> makes sense.
>>> Yeah, that would make sense.
>>
>> Thanks!  Here's a webrev with the renaming:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev
>>
>> and tested.
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (not as big as it looks) 8184994: Add Dictionary size logging and jcmd

coleen.phillimore
In reply to this post by coleen.phillimore

Hi Serguei,  I just checked it in.   Luckily one of the platforms
complained about the double ;; so I fixed it.
Thanks!
Coleen

On 8/2/17 11:51 AM, [hidden email] wrote:

> Hi Coleen,
>
> It looks good.
>
> Minor:
>
> http://cr.openjdk.java.net/~coleenp/8184994.02/webrev/src/share/vm/classfile/placeholders.hpp.udiff.html
> - void print() const PRODUCT_RETURN;
> + void print_entry(outputStream* st) const;;
>   Extra ';'
>
>
> Thanks,
> Serguei
>
>
> On 8/1/17 10:32, [hidden email] wrote:
>>
>>
>> On 8/1/17 1:19 PM, Aleksey Shipilev wrote:
>>> On 08/01/2017 07:14 PM, [hidden email] wrote:
>>>>> *) systemDictionary.cpp: in SystemDictionary::dump, the
>>>>> if(verbose) condition seems inverted. Should
>>>>> dump tables when verbose?
>>>> I kept the name dump_table from the VM.stringtable and
>>>> VM.symboltable dcmd code, when the function
>>>> really prints hashtable statistics.   I could rename that function
>>>> to be print_table_statistics()
>>>> instead.  And change ClassLoaderDataGraph::dump_dictionary to
>>>> print_dictionary_statistics.  If that
>>>> makes sense.
>>> Yeah, that would make sense.
>>
>> Thanks!  Here's a webrev with the renaming:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8184994.02/webrev
>>
>> and tested.
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>
>

Loading...