<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=us-ascii"><meta name=Generator content="Microsoft Word 12 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New","serif";
color:black;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0cm;
margin-right:0cm;
margin-bottom:0cm;
margin-left:36.0pt;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.changed
{mso-style-name:changed;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.EmailStyle22
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:944725203;
mso-list-type:hybrid;
mso-list-template-ids:-1404511362 -40583994 69009411 69009413 69009409 69009411 69009413 69009409 69009411 69009413;}
@list l0:level1
{mso-level-start-at:0;
mso-level-number-format:bullet;
mso-level-text:-;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;
font-family:"Calibri","sans-serif";
mso-fareast-font-family:Calibri;
mso-bidi-font-family:"Times New Roman";}
@list l0:level2
{mso-level-tab-stop:72.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level3
{mso-level-tab-stop:108.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level4
{mso-level-tab-stop:144.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level5
{mso-level-tab-stop:180.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level6
{mso-level-tab-stop:216.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level7
{mso-level-tab-stop:252.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level8
{mso-level-tab-stop:288.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
@list l0:level9
{mso-level-tab-stop:324.0pt;
mso-level-number-position:left;
text-indent:-18.0pt;}
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body bgcolor=white lang=SV link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='color:#1F497D'>Hi Per,<o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Thanks for taking a look!<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Inline.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><br>Cheers<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Markus<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'>From:</span></b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext'> Per Liden <br><b>Sent:</b> den 18 november 2013 13:22<br><b>To:</b> Markus Gronlund; hotspot-runtime-dev@openjdk.java.net; hotspot-gc-dev@openjdk.java.net; serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net<br><b>Subject:</b> Re: RFR(M): 8028128: Add a type safe alternative for working with counter based data<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>Hi Markus,<br><br>Looks like a nice cleanup. Naming it always hard, so I'll avoid that topic ;) A few other minor comments, feel free to ignore.<o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>[MG] i agree on the naming parts. I also agree that the current names are not the best possible. I have assumed they will change.<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><pre><span lang=EN-US> </span>101 class TicksToTimeHelper : public AllStatic {<o:p></o:p></pre><pre> 102 public:<o:p></o:p></pre><pre> 103 enum Unit {<o:p></o:p></pre><pre> 104 SECONDS = 1,<o:p></o:p></pre><pre> 105 MILLISECONDS = 1000<o:p></o:p></pre><pre> 106 };<o:p></o:p></pre><pre> 107 static double seconds(const Tickspan& span);<o:p></o:p></pre><pre> 108 static jlong milliseconds(const Tickspan& span);<o:p></o:p></pre><pre> 109 };<o:p></o:p></pre><pre><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></pre><p class=MsoNormal style='margin-bottom:12.0pt'>Do we really want a separate class for this instead of having Tickspan::seconds() and Tickspan::milliseconds()? If moved inside Tickspan, maybe value() should be ticks() to be consistent (and I guess Ticks::value() -> Ticks::ticks() would follow)?<o:p></o:p></p><p class=MsoNormal style='margin-bottom:12.0pt'><span lang=EN-US style='color:#1F497D'>[MG] In this particular case I would prefer to keep the convenience functions non-member, non-friends since a convenience member function really does not add anything extra that cannot be retrieved from outside the type. I also think that keeping the convenience functions outside/decoupled from the type itself increases encapsulation and increases flexibility – esp. for this case very much so where the convenience functions for at ticks/time representation is very much open ended; I just whacked together a seconds() and milliseconds(), but what about microseconds(), nanoseconds() and other timing convenience functions that might be needed further? I think keeping it outside increases the flexibility to add convenience functions without affecting the primary type.<o:p></o:p></span></p><pre><span lang=EN-US> </span>43 inline bool operator!=(const Tickspan& lhs, const Tickspan& rhs) {<o:p></o:p></pre><pre> 44 return !operator==(lhs,rhs);<o:p></o:p></pre><pre> 45 }<o:p></o:p></pre><p class=MsoNormal style='margin-bottom:12.0pt'>In many of the operators you have code like the example above. Is there a reason you want to spell it out like that instead of just "return !(lhs == rhs);"?<o:p></o:p></p><p class=MsoNormal style='margin-bottom:12.0pt'><span lang=EN-US style='color:#1F497D'>[MG] I use this way of writing to make it explicit that a particular non-member operator is implemented in terms of another non-member operator. This point of view might be considered a bit too “implementation” centric, but I have found it useful sometimes as an easy way to directly point to the implementation – granted, for a case such as !(lhs == rhs) that might already be obvious….<o:p></o:p></span></p><pre><span class=changed>145 void register_gc_phase_start(const char* name, const Ticks& time = Ticks::now());</span><o:p></o:p></pre><p class=MsoNormal><br>There are quite a few places where you've added default values like the one above. But as far as I can tell most (I think I saw at least one exception in there) of these functions are either always called without the argument or always called with the argument. Seems a bit unnecessary to provide this flexibility when it's never used (as it opens up for incorrect use of the interface)?<span style='color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>[MG] Most of the default parameters are there in order to avoid polluting all the GC collectors with the knowledge about the Ticks/Tickspan types – I can encapsulate the usages only to gcTrace and gcTimer modules. You are right about this for the register_gc_phase_* functions though – they always pass a parameter, providing a default parameter for them might be a bit gratuitous. I will update the phase functions accordingly:<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal style='text-autospace:none'><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> </span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>void</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> register_gc_phase_start(</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>const</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> </span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>char</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'>* name, </span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>const</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> Ticks& time);<o:p></o:p></span></p><p class=MsoNormal style='text-autospace:none'><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> </span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>void</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> register_gc_phase_end(</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:blue'>const</span><span lang=EN-US style='font-size:9.5pt;font-family:Consolas;color:windowtext'> Ticks& time);<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>Thanks Per!<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=EN-US style='color:#1F497D'>I will send out an updated webrev.</span><span lang=EN-US><br><br>cheers,<br>/Per<br><br>On 2013-11-12 12:17, Markus Gronlund wrote:</span><span lang=EN-US style='color:#1F497D'><o:p></o:p></span></p></div><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal><span lang=EN-US>Greetings,<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US> <o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US>Kindly asking for reviews for the following changeset:<o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US> <o:p></o:p></span></p><p class=MsoNormal><span lang=EN-US>Webrev: </span><a href="http://cr.openjdk.java.net/%7Emgronlun/8028128/webrev01/">http://cr.openjdk.java.net/~mgronlun/8028128/webrev01/</a> <o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Bug: </span><a href="https://bugs.openjdk.java.net/browse/JDK-8028128"><span lang=EN-US>https://bugs.openjdk.java.net/browse/JDK-8028128</span></a><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><b>Description:</b><o:p></o:p></p><p class=MsoNormal> <o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Currently, when working with counter based data, the primary data type representation in the VM are raw jlong's. jlong's are employed to represent both: <br><br>1. a single instance in time, for example a "now" timestamp <br>2. a time duration, a timespan, for example the duration between "now" - "then" <br><br>Having a raw jlong type represent both of these shifts the responsibility to the programmer to always ensure the correct validity of the counter data, without any assistance either at compile time or at runtime. <br><br>The event based tracing framework relies heavily on counter based data in order to keep track of time and timing related information and we have seen counter data errors where the representation sent to the tracing framework has submitted the wrong time, i.e. a timespan when a time instant was expected and vice versa. <br><br>We should therefore add an alternative to jlongs for representing counter based data. We should enlist the help of the type system to enforce the correct data types at compile time as well as introduce strong runtime asserts in order to help with the correct usage of time and to reduce the number of potential bugs. <br><br>I will therefore add two new types in src/share/vm/utilities: <br><br>1. "Ticks" - represents a single counter based timestamp, a single instance in time.<br>2. "Tickspan" - represents a counter based duration, a timespan, between two "Ticks" </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Please see the added files:</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>-<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]><span lang=EN-US>src/share/vm/utilities/ticks.hpp</span><o:p></o:p></p><p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>-<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]><span lang=EN-US>src/share/vm/utilities/ticks.inline.hpp</span><o:p></o:p></p><p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo2'><![if !supportLists]><span style='mso-list:Ignore'>-<span style='font:7.0pt "Times New Roman"'> </span></span><![endif]><span lang=EN-US>src/share/vm/utilities/ticks.cpp</span><o:p></o:p></p><p class=MsoListParagraph><span lang=EN-US> </span><o:p></o:p></p><p class=MsoListParagraph style='margin-left:0cm'><span lang=EN-US>I have also updated some of the existing event based tracing "event sites" to employ usage of these new types as well as updated a few places in the event based tracing framework implementation to accommodate these types.</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal><b><span lang=EN-US>Testing completed:</span></b><o:p></o:p></p><p class=MsoNormal><span lang=EN-US> </span><o:p></o:p></p><p class=MsoNormal>nsk.stress.testlist (UTE/Aurora)<o:p></o:p></p><p class=MsoNormal><span lang=EN-US>GC nightlies (Aurora)</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>gc/gctests/* (UTE/Aurora)</span><o:p></o:p></p><p class=MsoNormal>nsk.gc.testlist (UTE/Aurora)<o:p></o:p></p><p class=MsoNormal><span lang=EN-US>SpecJBB2013 (no regression)</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>SpecJBB2005 (no regression)</span><o:p></o:p></p><p class=MsoNormal><span lang=EN-US>Kitchensink (locally Windows) runs for +> 9 days (still running…)<br><br>Thanks in advance<br>Markus</span><o:p></o:p></p></blockquote><p class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New Roman","serif"'><o:p> </o:p></span></p></div></body></html>