RFR (M) JDK-6479360 - detailed dumping of class size statistics

Coleen Phillimore coleen.phillimore at oracle.com
Thu Jan 24 14:55:32 PST 2013 

Ioi,
This looks even better.  Thanks for making these other changes.
Coleen

On 01/24/2013 02:52 PM, Ioi Lam 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/20130124/029e739b/attachment-0001.html

More information about the hotspot-runtime-dev mailing list