<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">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.<br>
<pre> 101 class TicksToTimeHelper : public AllStatic {
102 public:
103 enum Unit {
104 SECONDS = 1,
105 MILLISECONDS = 1000
106 };
107 static double seconds(const Tickspan& span);
108 static jlong milliseconds(const Tickspan& span);
109 };</pre>
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)?<br>
<br>
<pre> 43 inline bool operator!=(const Tickspan& lhs, const Tickspan& rhs) {
44 return !operator==(lhs,rhs);
45 }</pre>
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);"?<br>
<br>
<pre><span class="changed">145 void register_gc_phase_start(const char* name, const Ticks& time = Ticks::now());</span></pre>
<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)?<br>
<br>
cheers,<br>
/Per<br>
<br>
On 2013-11-12 12:17, Markus Gronlund wrote:<br>
</div>
<blockquote cite="mid:061fa021-3b40-4161-aecd-a946c0ddde3e@default"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<meta name="Generator" content="Microsoft Word 12 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@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;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
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;}
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";}
span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Calibri","sans-serif";
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;}
@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";}
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]-->
<div class="WordSection1">
<p class="MsoNormal">Greetings,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></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">Webrev: <a moz-do-not-send="true"
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
moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8028128"><span
lang="EN-US">https://bugs.openjdk.java.net/browse/JDK-8028128</span></a><span
lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><b>Description:<o:p></o:p></b></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="color:black" 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" <o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">Please
see the added files:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoListParagraph"
style="text-indent:-18.0pt;mso-list:l0 level1 lfo1"><!--[if !supportLists]--><span
lang="EN-US"><span style="mso-list:Ignore">-<span
style="font:7.0pt "Times New Roman"">
</span></span></span><!--[endif]--><span lang="EN-US">src/share/vm/utilities/ticks.hpp<o:p></o:p></span></p>
<p class="MsoListParagraph"
style="text-indent:-18.0pt;mso-list:l0 level1 lfo1"><!--[if !supportLists]--><span
lang="EN-US"><span style="mso-list:Ignore">-<span
style="font:7.0pt "Times New Roman"">
</span></span></span><!--[endif]--><span lang="EN-US">src/share/vm/utilities/ticks.inline.hpp<o:p></o:p></span></p>
<p class="MsoListParagraph"
style="text-indent:-18.0pt;mso-list:l0 level1 lfo1"><!--[if !supportLists]--><span
lang="EN-US"><span style="mso-list:Ignore">-<span
style="font:7.0pt "Times New Roman"">
</span></span></span><!--[endif]--><span lang="EN-US">src/share/vm/utilities/ticks.cpp<o:p></o:p></span></p>
<p class="MsoListParagraph"><span style="color:black"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoListParagraph" style="margin-left:0cm"><span
style="color:black" 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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><b><span style="color:black" lang="EN-US">Testing
completed:<o:p></o:p></span></b></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:black">nsk.stress.testlist
(UTE/Aurora)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">GC
nightlies (Aurora)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">gc/gctests/*
(UTE/Aurora)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black">nsk.gc.testlist
(UTE/Aurora)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">SpecJBB2013
(no regression)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">SpecJBB2005
(no regression)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:black" lang="EN-US">Kitchensink
(locally Windows) runs for +> 9 days (still running…)<br>
<br>
Thanks in advance<br>
Markus<o:p></o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>