RFR (M) JDK-6479360 - detailed dumping of class size statistics [last call]

Staffan Larsen staffan.larsen at oracle.com
Fri Jan 25 11:58:42 PST 2013 

Looks good! Thank you.

/Staffan

On 25 jan 2013, at 20:45, Ioi Lam <ioi.lam at oracle.com> wrote:

> Hi Steffan,
> 
> Thanks for the feedback. I have updated the patch according to your suggestions.
> 
>      http://cr.openjdk.java.net/~iklam/6479360/class_stats_014/
> 
> The only changes compared to the previous webrev are in diagnosticCommand.cpp and diagnosticCommand.hpp.
> 
> If everyone is happy, I will try to push the changes via Karen.
> 
> Thanks
> - Ioi
> 
> On 01/24/2013 11:43 PM, Staffan Larsen wrote:
>> Hi Ioi,
>> 
>> I was looking at the change in diagnosticCommand.cpp. In your change the new command is only available when running with -XX:+ UnlockDiagnosticVMOptions. I was wondering if it would make sense to instead always expose the diagnostic command, and check UnlockDiagnosticVMOptions in the execute() method. The execute() method can then print a helpful message saying that you need to enable UnlockDiagnosticVMOptions if it isn't enabled. This would make it easier to discover the availability of the diagnostic command and also give users a hint on how to enable it.
>> 
>> Thanks,
>> /Staffan
>> 
>> On 24 jan 2013, at 20:52, Ioi Lam <ioi.lam at oracle.com> wrote:
>> 
>>> Hi,
>>> 
>>> I have updated the patch:
>>> 
>>>     http://cr.openjdk.java.net/~iklam/6479360/class_stats_012/
>>> 
>>> [1] Removed FOREACH_COLUMN and SELECTED per Harold
>>> [2] Modified HeapInspection constructor per Coleen.
>>> [3] Renamed a few columns in the output to match the capitalizations
>>>     in source code:
>>>     Constmethod -> ConstMethod
>>>     Methoddata -> MethodData,  etc
>>> 
>>> Thanks!
>>> - Ioi
>>> 
>>> 
>>> On 01/24/2013 08:34 AM, harold seigel wrote:
>>>> Hi Ioi,
>>>> 
>>>> I think we want to get away from using lots of macros, such as these that were added to heapinspection.cpp:
>>>> +#define FOREACH_COLUMN(c) \
>>>>  + for (int c=0; c<KlassSizeStats::_num_columns; c++)
>>>> +
>>>> +#define SELECTED(c) selected_columns_table[c]
>>>> Would you consider removing them?
>>>> 
>>>> Thanks, Harold
>>>> 
>>>> On 1/23/2013 8:24 PM, Coleen Phillimore wrote:
>>>>> 
>>>>> 
>>>>> In vmGCOperations.cpp the type HeapInspection inspect looks like the only initialization of these fields is in the set functions.
>>>>> 
>>>>> It would be cleaner to declare the constructor to take the initial values in the declaration, ie:
>>>>> 
>>>>>     HeapInspection inspect(_csv_format, _print_help, _print_class_stats, _colmns);
>>>>> 
>>>>> and then you can delete the setter functions.
>>>>> 
>>>>> The rest of the code looks good.
>>>>> Coleen
>>>>> 
>>>>> On 1/23/2013 6:59 PM, Karen Kinnear wrote:
>>>>>> 
>>>>>> Ioi,
>>>>>> 
>>>>>> Thanks for finding that - I had missed that. That makes a lot of sense. Thank you.
>>>>>> So - you have my approval for checking this in. You need one more code reviewer.
>>>>>> 
>>>>>> thanks,
>>>>>> Karen
>>>>>> 
>>>>>> On Jan 23, 2013, at 6:57 PM, Ioi Lam wrote:
>>>>>> 
>>>>>>> Karen,
>>>>>>> 
>>>>>>> Thanks for the review.
>>>>>>> 
>>>>>>> hotspot/make/excludeSrc.make already excludes the entire file if INCLUDE_SERVICES=0. I verified that heapInspection.o does not exist for the "linux_i486_minimal1" build, which has INCLUDE_SERVICES=0.
>>>>>>> 
>>>>>>> ifeq ($(INCLUDE_SERVICES), false)
>>>>>>>       CXXFLAGS += -DINCLUDE_SERVICES=0
>>>>>>>       CFLAGS += -DINCLUDE_SERVICES=0
>>>>>>> 
>>>>>>>       Src_Files_EXCLUDE += heapDumper.cpp heapInspection.cpp \
>>>>>>>     attachListener_linux.cpp attachListener.cpp
>>>>>>> endif
>>>>>>> 
>>>>>>> - Ioi
>>>>>>> 
>>>>>>> On 01/23/2013 03:31 PM, Karen Kinnear wrote:
>>>>>>>> Ioi,
>>>>>>>> 
>>>>>>>> Thank you for the cleanups. Looks good. Ok to check in.
>>>>>>>> 
>>>>>>>> Or - if you are up to adding INCLUDE_SERVICES around a bit more - 
>>>>>>>> 
>>>>>>>> including heapInspection.cpp/hpp (at least that entire table of
>>>>>>>> strings) and print_class_stats and the call to it. The goal is to reduce memory
>>>>>>>> usage when not needed.
>>>>>>>> 
>>>>>>>> And thank you for testing with that flag off.
>>>>>>>> 
>>>>>>>> thanks,
>>>>>>>> Karen
>>>>>>>> 
>>>>>>>> (p.s. you might ask Jiangli for a code review)
>>>>>>>> 
>>>>>>>> On Jan 23, 2013, at 5:18 PM, Ioi Lam wrote:
>>>>>>>> 
>>>>>>>>> Thanks to Karen for the feedback. I have updated the webrev:
>>>>>>>>> 
>>>>>>>>>     http://cr.openjdk.java.net/~iklam/6479360/class_stats_011/
>>>>>>>>> 
>>>>>>>>> Changes since last time
>>>>>>>>> 
>>>>>>>>>     + Cleaned up typos found by Karen
>>>>>>>>> 
>>>>>>>>>     + Use JULONG_FORMAT. Remove all uses of PRIu64 and 
>>>>>>>>>       UINT64_FORMAT which may be unportable on MacOS (see 7102489).
>>>>>>>>> 
>>>>>>>>>     + Updated copyright to 2013
>>>>>>>>> 
>>>>>>>>>     + Added more #if INCLUDE_SERVICES. GC.class_histogram and 
>>>>>>>>>       GC.class_stats are no longer accessible by jcmd if
>>>>>>>>>       INCLUDE_SERVICES=0.
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> - Ioi
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On 01/22/2013 02:07 PM, Karen Kinnear wrote:
>>>>>>>>>> Ioi,
>>>>>>>>>> 
>>>>>>>>>> Thank you for doing this. I like the way you have the size collection in the file
>>>>>>>>>> with the metadata definitions.
>>>>>>>>>> 
>>>>>>>>>> 1. diagnosticCommand.hpp line 200 - inspeactheap -> inspectheap
>>>>>>>>>> 
>>>>>>>>>> 2. please update copyrights on all files you changed to 2013
>>>>>>>>>> 
>>>>>>>>>> 3. I sent an email to the embedded folks to find out if they want this
>>>>>>>>>>    under INCLUDE_SERVICES.
>>>>>>>>>>    If they do ...
>>>>>>>>>> 
>>>>>>>>>>    - did you build with and without INCLUDE_SERVICES?
>>>>>>>>>>    What happens when you build with it off and run the jcmd ?
>>>>>>>>>>    I am confused that in diagnosticCommand.cpp ClassStatsDcmd is available without
>>>>>>>>>>    INCLUDE_SERVICES, but the actual details below the collect_statistics are not?
>>>>>>>>>>    Did you talk to the embedded team about whether they want this or not?
>>>>>>>>>> 
>>>>>>>>>>    - I would recommend that include files also add the new methods conditional
>>>>>>>>>>   on INCLUDE_SERVICES - you did that in some of them, but I think you missed:
>>>>>>>>>>   method.hpp, methodData.hpp, constantPool.hpp, constMethod.hpp, annotations.hpp
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> 4. heapInspection.hpp line 56: is InstBytesi supposed to be InstBytes?
>>>>>>>>>> 
>>>>>>>>>> 5. heapInspection.hpp line 122 - you have some funny "\" - is that intentional?
>>>>>>>>>> 
>>>>>>>>>> 6. INT64_FORMAT: I think Harold just put back a change for julong handling
>>>>>>>>>> that switched to using JULONG_FORMAT- see KlassInfoHisto::print_class_stats - 2 places
>>>>>>>>>> (to ensure this also works on Mac OS/X)
>>>>>>>>>> 
>>>>>>>>>> thanks,
>>>>>>>>>> Karen
>>>>>>>>>> 
>>>>>>>>>> On Jan 16, 2013, at 7:39 PM, Ioi Lam wrote:
>>>>>>>>>> 
>>>>>>>>>>> Please review:
>>>>>>>>>>>     http://cr.openjdk.java.net/~acorn/class_stats_010/
>>>>>>>>>>> 
>>>>>>>>>>> Bug: RFE: PrintClassHistogram improvements
>>>>>>>>>>>     https://jbs.oracle.com/bugs/browse/JDK-6479360
>>>>>>>>>>> 
>>>>>>>>>>> Sponsor: Karen Kinnear
>>>>>>>>>>> 
>>>>>>>>>>> Summary:
>>>>>>>>>>> 
>>>>>>>>>>> A new diagnostic command "jcmd GC.class_stats" is added. The user
>>>>>>>>>>> can invoke this command to connect to a live JVM and dump a detailed
>>>>>>>>>>> report of size statistics of all loaded classes (including array
>>>>>>>>>>> classes and anonymous classes).
>>>>>>>>>>> 
>>>>>>>>>>>     ==========SYNOPSIS===================================
>>>>>>>>>>>     $ jcmd $PID help GC.class_stats
>>>>>>>>>>>     Provide statistics about Java class meta data.
>>>>>>>>>>>     Impact: High: Depends on Java heap size and content.
>>>>>>>>>>> 
>>>>>>>>>>>     Syntax : GC.class_stats [options] [<columns>]
>>>>>>>>>>> 
>>>>>>>>>>>     Arguments:
>>>>>>>>>>> 	columns : [optional] Comma-separated list of all the columns to
>>>>>>>>>>>                   show. If not specified, the following columns are shown:
>>>>>>>>>>>                   InstBytes,KlassBytes,CpAll,annotations,MethodCount,Bytecodes,
>>>>>>>>>>>                   MethodAll,ROAll,RWAll,Total (STRING, no default value)
>>>>>>>>>>> 
>>>>>>>>>>>     Options: (options must be specified using the <key> or <key>=<value> syntax)
>>>>>>>>>>> 	-all : [optional] Show all columns (BOOLEAN, false)
>>>>>>>>>>> 	-csv : [optional] Print in CSV (comma-separated values) format for 
>>>>>>>>>>>                 spreadsheets (BOOLEAN, false)
>>>>>>>>>>> 	-help : [optional] Show meaning of all the columns (BOOLEAN, false)
>>>>>>>>>>>     ======================================================
>>>>>>>>>>> 
>>>>>>>>>>> By default, the output is human-readable tabulated text format. The
>>>>>>>>>>> user can also select CSV format (comma-separated values) to be
>>>>>>>>>>> used with spreadsheets.
>>>>>>>>>>> 
>>>>>>>>>>> A large number of columns are available. By default, a few "summary 
>>>>>>>>>>> columns" (e.g., size of Klass objects, total read-only data, etc) 
>>>>>>>>>>> are printed. The user can also manually select the columns.
>>>>>>>>>>> 
>>>>>>>>>>> Please see the attachments in the bug for sample output, as well as
>>>>>>>>>>> a listing of all the columns and their meanings:
>>>>>>>>>>> 
>>>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6479360?focusedCommentId=13290360&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13290360
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Impact:
>>>>>>>>>>> 
>>>>>>>>>>> If this diagnostic command is not used, there should be no
>>>>>>>>>>> impact to the JVM's execution, except for
>>>>>>>>>>> 
>>>>>>>>>>>    + libjvm.so footprint increase (about 10KB larger on Linux/x64)
>>>>>>>>>>>    + one additional virtual method in Klass.
>>>>>>>>>>> 
>>>>>>>>>>> This feature is excluded when INCLUDE_SERVICES=0 (e.g., 
>>>>>>>>>>> embedded builds).
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Tests run:
>>>>>>>>>>> 
>>>>>>>>>>> + JPRT -- (hotspot only) to verify build-ability
>>>>>>>>>>> 
>>>>>>>>>>> + Manually ran "jcmd $PID help GC.class_stats" on Linux/x64
>>>>>>>>>>> 
>>>>>>>>>>> + I intend to add a new testcase once this is committed:
>>>>>>>>>>>   https://jbs.oracle.com/bugs/browse/JDK-8006413
>>>>>>>>>>>   Add utility classes for writing better multiprocess tests in jtreg
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Ioi
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130125/32086c2c/attachment-0001.html

More information about the hotspot-runtime-dev mailing list