<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#ffffff" text="#000000">
    <br>
    In vmGCOperations.cpp the type HeapInspection inspect looks like the
    only initialization of these fields is in the set functions.<br>
    <br>
    It would be cleaner to declare the constructor to take the initial
    values in the declaration, ie:<br>
    <br>
    &nbsp;&nbsp;&nbsp; HeapInspection inspect(_csv_format, _print_help,
    _print_class_stats, _colmns);<br>
    <br>
    and then you can delete the setter functions.<br>
    <br>
    The rest of the code looks good.<br>
    Coleen<br>
    <br>
    On 1/23/2013 6:59 PM, Karen Kinnear wrote:
    <blockquote
      cite="mid:B9ACC30B-12C6-417B-AB26-553E03CCCBCA@oracle.com"
      type="cite">Ioi,
      <div><br>
      </div>
      <div>Thanks for finding that - I had missed that. That makes a lot
        of sense. Thank you.</div>
      <div>So - you have my approval for checking this in. You need one
        more code reviewer.</div>
      <div><br>
      </div>
      <div>thanks,</div>
      <div>Karen</div>
      <div><br>
        <div>
          <div>On Jan 23, 2013, at 6:57 PM, Ioi Lam wrote:</div>
          <br class="Apple-interchange-newline">
          <blockquote type="cite">
            <meta content="text/html; charset=ISO-8859-1"
              http-equiv="Content-Type">
            <div bgcolor="#FFFFFF" text="#000000">
              <div class="moz-cite-prefix">Karen,<br>
                <br>
                Thanks for the review.<br>
                <br>
                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.<br>
                <br>
                ifeq ($(INCLUDE_SERVICES), false)<br>
                &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; CXXFLAGS += -DINCLUDE_SERVICES=0<br>
                &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; CFLAGS += -DINCLUDE_SERVICES=0<br>
                <br>
                &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; Src_Files_EXCLUDE += heapDumper.cpp
                heapInspection.cpp \<br>
                &nbsp;&nbsp;&nbsp; attachListener_linux.cpp attachListener.cpp<br>
                endif<br>
                <br>
                - Ioi<br>
                <br>
                On 01/23/2013 03:31 PM, Karen Kinnear wrote:<br>
              </div>
              <blockquote
                cite="mid:2AABC1FB-8A4B-4F71-980F-0158C1A83BF8@oracle.com"
                type="cite">Ioi,
                <div><br>
                </div>
                <div>Thank you for the cleanups. Looks good. Ok to check
                  in.</div>
                <div><br>
                </div>
                <div>Or - if you are up to adding INCLUDE_SERVICES
                  around a bit more -&nbsp;</div>
                <div><br>
                </div>
                <div>including heapInspection.cpp/hpp (at least that
                  entire table of</div>
                <div>strings) and print_class_stats and the call to it.
                  The goal is to reduce memory</div>
                <div>usage when not needed.</div>
                <div><br>
                </div>
                <div>And thank you for testing with that flag off.</div>
                <div><br>
                </div>
                <div>thanks,</div>
                <div>Karen</div>
                <div><br>
                </div>
                <div>(p.s. you might ask Jiangli for a code review)</div>
                <div><br>
                  <div>
                    <div>On Jan 23, 2013, at 5:18 PM, Ioi Lam wrote:</div>
                    <br class="Apple-interchange-newline">
                    <blockquote type="cite">
                      <meta content="text/html; charset=ISO-8859-1"
                        http-equiv="Content-Type">
                      <div bgcolor="#FFFFFF" text="#000000">
                        <div class="moz-cite-prefix"><tt>Thanks to Karen
                            for the feedback. I have updated the webrev:</tt><tt><br>
                          </tt><tt><br>
                          </tt><tt>&nbsp;&nbsp;&nbsp; <a moz-do-not-send="true"
                              class="moz-txt-link-freetext"
                              href="http://cr.openjdk.java.net/%7Eiklam/6479360/class_stats_011/">http://cr.openjdk.java.net/~iklam/6479360/class_stats_011/</a></tt><tt><br>
                          </tt><tt><br>
                            Changes since last time<br>
                            <br>
                            &nbsp;&nbsp;&nbsp; + Cleaned up typos found by Karen<br>
                            <br>
                            &nbsp;&nbsp;&nbsp; + Use JULONG_FORMAT. Remove all uses of
                            PRIu64 and <br>
                            &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; UINT64_FORMAT which may be unportable
                            on MacOS (see 7102489</tt><tt>).</tt><tt><br>
                            <br>
                            &nbsp;&nbsp;&nbsp; + Updated copyright to 2013<br>
                            <br>
                            &nbsp;&nbsp;&nbsp; + Added more #if INCLUDE_SERVICES.
                            GC.class_histogram and <br>
                            &nbsp; &nbsp; &nbsp; <tt>GC.class_stats are no longer
                              accessible by jcmd if<br>
                              &nbsp; &nbsp; &nbsp; <tt>INCLUDE_SERVICES=0.</tt></tt></tt><br>
                          <tt><br>
                            Thanks<br>
                          </tt><tt>- Ioi<br>
                          </tt><br>
                          <br>
                          On 01/22/2013 02:07 PM, Karen Kinnear wrote:<br>
                        </div>
                        <blockquote
                          cite="mid:CE748035-F410-49A1-A070-0C00CCDF7A8C@oracle.com"
                          type="cite">Ioi,
                          <div><br>
                          </div>
                          <div>Thank you for doing this. I like the way
                            you have the size collection in the file</div>
                          <div>with the metadata definitions.</div>
                          <div><br>
                          </div>
                          <div>1. diagnosticCommand.hpp line 200 -
                            inspeactheap -&gt; inspectheap</div>
                          <div><br>
                          </div>
                          <div>2. please update copyrights on all files
                            you changed to 2013</div>
                          <div><br>
                          </div>
                          <div>3. I sent an email to the embedded folks
                            to find out if they want this</div>
                          <div>&nbsp; &nbsp;under INCLUDE_SERVICES.</div>
                          <div>&nbsp; &nbsp;If they do ...</div>
                          <div><br>
                          </div>
                          <div>&nbsp; &nbsp;- did you build with and without
                            INCLUDE_SERVICES?</div>
                          <div>&nbsp; &nbsp;What happens when you build with it
                            off and run the jcmd ?</div>
                          <div>&nbsp; &nbsp;I am confused that in
                            diagnosticCommand.cpp ClassStatsDcmd is
                            available without</div>
                          <div>&nbsp; &nbsp;INCLUDE_SERVICES, but the actual
                            details below the collect_statistics are
                            not?</div>
                          <div>&nbsp; &nbsp;Did you talk to the embedded team
                            about whether they want this or not?</div>
                          <div><br>
                          </div>
                          <div>&nbsp; &nbsp;- I would recommend that include files
                            also add the new methods conditional</div>
                          <div>&nbsp; on INCLUDE_SERVICES - you did that in
                            some of them, but I think you missed:</div>
                          <div>&nbsp; method.hpp,
                            methodData.hpp,&nbsp;constantPool.hpp,
                            constMethod.hpp, annotations.hpp</div>
                          <div><br>
                          </div>
                          <div><br>
                          </div>
                          <div>4. heapInspection.hpp line 56: is
                            InstBytesi supposed to be InstBytes?</div>
                          <div><br>
                          </div>
                          <div>5. heapInspection.hpp line 122 - you have
                            some funny "\" - is that intentional?</div>
                          <div><br>
                          </div>
                          <div>6. INT64_FORMAT: I think Harold just put
                            back a change for julong handling</div>
                          <div>that switched to using JULONG_FORMAT- see
                            KlassInfoHisto::print_class_stats - 2 places</div>
                          <div>(to ensure this also works on Mac OS/X)</div>
                          <div><br>
                          </div>
                          <div>thanks,</div>
                          <div>Karen</div>
                          <div><br>
                            <div>
                              <div>On Jan 16, 2013, at 7:39 PM, Ioi Lam
                                wrote:</div>
                              <br class="Apple-interchange-newline">
                              <blockquote type="cite">
                                <meta http-equiv="content-type"
                                  content="text/html;
                                  charset=ISO-8859-1">
                                <div bgcolor="#FFFFFF" text="#000000">
                                  <div class="moz-text-html"
                                    lang="x-western">
                                    <pre>Please review:
    <a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eacorn/class_stats_010/">http://cr.openjdk.java.net/~acorn/class_stats_010/</a>

Bug: RFE: PrintClassHistogram improvements
    <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-6479360">https://jbs.oracle.com/bugs/browse/JDK-6479360</a>

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] [&lt;columns&gt;]

    Arguments:
        columns : [optional] Comma-separated list of all the columns to
                 &nbsp;show. If not specified, the following columns are shown:
                 &nbsp;InstBytes,KlassBytes,CpAll,annotations,MethodCount,Bytecodes,
                  MethodAll,ROAll,RWAll,Total (STRING, no default value)

    Options: (options must be specified using the &lt;key&gt; or &lt;key&gt;=&lt;value&gt; 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:

<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-6479360?focusedCommentId=13290360&amp;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13290360">https://jbs.oracle.com/bugs/browse/JDK-6479360?focusedCommentId=13290360&amp;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13290360</a>



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:
<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">  <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-8006413">https://jbs.oracle.com/bugs/browse/JDK-8006413</a>
  Add utility classes for writing better multiprocess tests in jtreg

Thanks,
Ioi
</pre>
                                  </div>
                                </div>
                              </blockquote>
                            </div>
                            <br>
                          </div>
                        </blockquote>
                        <br>
                      </div>
                    </blockquote>
                  </div>
                  <br>
                </div>
              </blockquote>
              <br>
            </div>
          </blockquote>
        </div>
        <br>
      </div>
    </blockquote>
  </body>
</html>