<div dir="ltr"><div dir="ltr"><div><br></div><div>Hi Tony,</div><div><br></div><div>I think this makes sense. I agree, the output as it is now can be confusing. Note that users can still be confused since users may simply not be aware that the default ClassSpace reserved size is 1G.</div><div><br></div><div>Can your change break scripts for folks parsing GC logs? I see that you only appended content and did not change the existing text, so I guess this is okay.<br></div><div><br class="gmail-Apple-interchange-newline"></div><div>---</div><div><br></div><div><div>Format and naming: Different forms are used in different places. In hs-err files, we print "Metaspace" and "Class Space", with the former being the total sum and the latter the class space part.</div><div><br></div><div>But in other corners, eg. in the jcmd VM.metaspace command, we print non-class space and class space and maybe the sum in addition. Usually non-class space is called "Non-Class".</div><div><br></div><div>I personally would prefer, in this case, the latter names: "Class Space" and "Non-Class Space"</div><br class="gmail-Apple-interchange-newline"></div><div><div>---<br></div></div><div><br></div><div>Note that for non-class space, reserved size can actually change when allocating and releasing metaspace, if VirtualSpaceNodes get purged or newly allocated. Hence total reserved size can change too. So we may want to print out reserved before and reserved after sizes. I guess that was also missing from the output before.</div><div><br></div><div>----</div><div><br></div><div>Since used = class used + nonclass used, I would simplify PreMetaspaceValues to _non_class_used and _class_used and calculate the sum on the fly.</div><div><br></div><div>---</div><div><br></div><div>You added a new dependency to heap.hpp into metaspace.cpp for the format string. Is this really necessary? Can you move this define to metaspace.cpp?</div><div><br></div><div>If not, you need at least add include heap.hpp (did it build this way without precompiled headers?)</div><div><br></div><div>----</div><div><br></div><div>The following issues are matters of taste, and I leave it up to you whether you want to change something:</div><div><br></div><div><div>-> I wince a bit at yet another metaspace-sizes-structure - we have already ClassLoaderMetaspaceStatistics and MetaspaceCounters, and maybe others. JFR may also do something similar. Would it be possible to reuse one of those? (I guess ClassLoaderMetaspaceStatistics is too fine granular and too expensive to collect.)</div><div><br></div></div><div>-> I would prefer the class PreMetaspaceValues to live inside an own header in metaspace folder, and in the metaspace namespace like we did for similar helpers.</div><div><br></div><div>-> I would take the "Pre" out of the name and the member names and just name it "Snapshot" and the members "used" and "class used".</div><div><br></div><div>-> I would probably have made PreMetaspaceValues a dumb structure and added a function "MetaspaceAux::get_dimensions()" instead of adding the implementation to the constructor and to the header file. At least I would prefer the ctor implementation to the cpp file.</div><div><br></div><div>Cheers, Thomas</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 8, 2019 at 4:20 PM Tony Printezis <<a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;"><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">Any chance of someone taking a look at this?</div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><a href="http://cr.openjdk.java.net/~tonyp/8223575/webrev.0/" target="_blank">http://cr.openjdk.java.net/~tonyp/8223575/webrev.0/</a></div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">As I said on the JIRA, users can be confused when they metaspace capacity shows as over 1G even though they asked for 128M. Breaking down the spaces, when coops are enabled, can address this confusion (and having the actual occupancy of each space separately is also a lot more informative and avoids guesswork). The output looks like this when coops are enabled:</div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div id="gmail-m_2409837340603532282bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><p style="margin:0px;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo"><span style="font-variant-ligatures:no-common-ligatures">[10.090s][info][gc,metaspace     ] GC(8) Metaspace: 2130K->2130K(1056768K) ClassMetadata: 1906K->1906K(8192K) ClassSpace: 223K->223K(1048576K)</span></p></div><div><br></div>and like this (i.e., what it was before the change) when coops are not enabled:<div><br></div><div><p style="margin:0px;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo"><span style="font-variant-ligatures:no-common-ligatures">[25.767s][info][gc,metaspace     ] GC(28) Metaspace: 8107K->8107K(10240K)</span></p></div><div><br></div><div>A couple of notes on the changes:</div><div><br></div><div>- I’m open to different suggestions for what to call ClassMetadata and ClassSpace. :-)</div><div>- I didn’t know where to put the HEAP_CHANGE_FORMAT and HEAP_CHANGE_FORMAT_ARGS macros, so that they can be easily shared, and I ended up putting them in heap.hpp. Let me know if there’s a better place for them.</div><div><br></div><div>Tony</div><div><br><div id="gmail-m_2409837340603532282bloop_sign_1557324722471365888" class="gmail-m_2409837340603532282bloop_sign"><div><br></div><div><div>—————</div><div>Tony Printezis | @TonyPrintezis | <a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a></div></div><div><br></div></div></div></div>
</blockquote></div>