RFR (M) JDK-6479360 - detailed dumping of class size statistics
harold seigel
harold.seigel at oracle.com
Thu Jan 24 08:34:39 PST 2013
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/3b003cec/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list