<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Mikael,<br>
<br>
Sorry for the confusion. I see what you mean.<br>
I missed that you made it consistent with the current style in
these files.<br>
Agree, that is the best you can do.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 11/26/12 2:37 PM, Mikael Vidstedt wrote:<br>
</div>
<blockquote cite="mid:50B3EF15.1080500@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<br>
Serguei,<br>
<br>
I'm confused. When I look at the latest webrev (<a
moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.02/">http://cr.openjdk.java.net/~mikael/8003879/webrev.02/</a>)
I believe the indentation is correct - correct being defined as
aligned on the first letter of the type name. Please clarify what
you expect if you do not agree.<br>
<br>
Thanks,<br>
Mikael<br>
<br>
<div class="moz-cite-prefix">On 11/26/2012 12:21 PM, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:50B3CF61.6010501@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Looks good.<br>
However, it'd be nice to fix the indentation commented by
David (use frames to notice it):<br>
<br>
Â
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<b>vmStructs_cms.hpp</b>:<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre><span class="changed">68 declare_type(AFLBinaryTreeDictionary, FreeBlockDictionary<FreeChunk>)</span></pre>
<br>
Â
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<b>jni.cpp:</b>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre>2110 declare_type(MetablockTreeDictionary, FreeBlockDictionary<Metablock>)</pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/22/12 5:04 PM, Mikael Vidstedt wrote:<br>
</div>
<blockquote cite="mid:50AECB9F.8070207@oracle.com" type="cite">
<br>
On 2012-11-22 16:35, David Holmes wrote: <br>
<blockquote type="cite">Hi Mikael, <br>
<br>
I prefer this as an internal test too. Though I don't
understand why execute_internal_vm_tests is in jni.cpp
rather than jvm.cpp - this is an enhancement to the VM
interface not the JNI interface isn't it? (Separate issue of
course). <br>
</blockquote>
<br>
I think we'll find that when we get a few more tests in there
it's time to look at breaking it out into a separate file
completely or do it in a completely different way, but that's
for another day. I do hope it's soon though. <br>
<br>
<blockquote type="cite">BTW the weird indentation makes more
sense viewing frames rather than diffs :) <br>
</blockquote>
<br>
Right :) <br>
<br>
<blockquote type="cite"> <br>
Looks good to go. <br>
</blockquote>
<br>
Thanks! Eagerly awaiting a second review! <br>
<br>
/Mikael <br>
<br>
<blockquote type="cite"> <br>
David <br>
<br>
On 23/11/2012 10:16 AM, Mikael Vidstedt wrote: <br>
<blockquote type="cite"> <br>
I gave it some more thought and decided to try what it
would look like <br>
having the test be part of the ExecuteInternalVMTests
family instead. I <br>
like this one better personally, but comments are
appreciated! <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.02">http://cr.openjdk.java.net/~mikael/8003879/webrev.02</a>
<br>
<br>
Cheers, <br>
Mikael <br>
<br>
On 2012-11-22 14:25, Mikael Vidstedt wrote: <br>
<blockquote type="cite"> <br>
New webrev available here: <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.01">http://cr.openjdk.java.net/~mikael/8003879/webrev.01</a>
<br>
<br>
Cheers, <br>
Mikael <br>
<br>
On 2012-11-22 14:19, Mikael Vidstedt wrote: <br>
<blockquote type="cite">On 2012-11-21 20:17, David
Holmes wrote: <br>
<blockquote type="cite">Hi Mikael, <br>
<br>
On 22/11/2012 11:42 AM, Mikael Vidstedt wrote: <br>
<blockquote type="cite"> <br>
Please review the below change. <br>
<br>
The change for 7045397 introduced a couple of
duplicate entries in the <br>
vmStructs::localHotSpotVMTypes array. This shows
up when using the <br>
jmap <br>
tool in a rather ugly way: <br>
</blockquote>
<snip> <br>
<br>
Other than an indentation problem (which I don't
think you <br>
introduced) this fixup seems fine. <br>
</blockquote>
<br>
For some reason that's the standard for declaring
types in that long <br>
list - aligning on the first letter of the type name.
I followed suit. <br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">In addition to removing the
two duplicated entries I also added a <br>
simple, naive runtime test to walk through and
make sure no type is <br>
repeated. The VMStructs::init only called in
debug_only so there's no <br>
startup overhead in product, but it may be better
to turn the test <br>
into <br>
a unit test and only running it as part of
ExecuteInternalVMTests. <br>
Feedback appreciated! <br>
</blockquote>
<br>
I have a few comments on this part: <br>
<br>
- Array indices should be int's not size_t (ie
signed not unsigned) <br>
</blockquote>
<br>
Will fix. <br>
<br>
<blockquote type="cite">- I can't clearly see how the
localHotSpotVMTypes array is declared <br>
or filled but I assume there is guaranteed to be a
sentinel entry at <br>
the end so that we don't index past the end? <br>
</blockquote>
<br>
Yeah. I'm not sure you want to know. It took me a few
minutes to <br>
figure it out. It turns out that the
GENERATE_VM_TYPE_LAST_ENTRY <br>
macro which generates the end marker is passed to
VM_TYPES, <br>
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last
one is allowed to <br>
actually allowed to call the macro and actually
generate the marker, <br>
meaning only the "implementation" of VM_TYPES_OS_CPU
actually has the <br>
call to last_entry(). Not sure why the end marker
isn't just <br>
explicitly added at the end instead, but I think I'll
queue that up <br>
for later. <br>
<br>
All in all, as far as I can tell there is one and only
one end marker <br>
and it is generated as part of the expansion of
VM_TYPES_OS_CPU. <br>
<br>
<blockquote type="cite">- assert(0, ...) should be
assert(false, ...) (as per style guide <br>
[1] ;-) ) <br>
</blockquote>
<br>
Will fix. <br>
<br>
<blockquote type="cite">That all said I'm not sure
this test belongs there, but I don't feel <br>
strongly either way. <br>
</blockquote>
<br>
Me neither :) <br>
<br>
Cheers, <br>
Mikael <br>
<br>
<blockquote type="cite"> <br>
Cheers, <br>
David <br>
<br>
[1] <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://wikis.oracle.com/display/HotSpotInternals/StyleGuide">https://wikis.oracle.com/display/HotSpotInternals/StyleGuide</a>
<br>
<br>
<blockquote type="cite"> <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Emikael/8003879/webrev.00/">http://cr.openjdk.java.net/~mikael/8003879/webrev.00/</a>
<br>
<br>
Cheers, <br>
Mikael <br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>