RFR (M) JDK-6479360 - detailed dumping of class size statistics [last call]
Ioi Lam
ioi.lam at oracle.com
Fri Jan 25 11:45:45 PST 2013
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
> <mailto: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/f4f2fdd8/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list