<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<font face="Times New Roman, Times, serif">Kim,<br>
<br>
</font><br>
<div class="moz-cite-prefix">On 11/26/2015 02:34 PM, Kim Barrett
wrote:<br>
</div>
<blockquote
cite="mid:DFB0D75F-3696-4640-A1AC-04C434E1FE9E@oracle.com"
type="cite">
<pre wrap="">On Nov 25, 2015, at 5:10 PM, Jon Masamitsu <a class="moz-txt-link-rfc2396E" href="mailto:jon.masamitsu@oracle.com"><jon.masamitsu@oracle.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
I have a new fix for this bug. My previous fix broke solaris-x86 (I
had not defined an early_initialize() for x86). This fix is slightly
smaller and has the virtue of moving the required initialization
closer to where it is used.
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~jmasa/8133023/webrev.03/index.html">http://cr.openjdk.java.net/~jmasa/8133023/webrev.03/index.html</a>
Testing: JPRT build on all platforms, checked by hand that the correct number
of GC worker threads are created on later Niagara platforms.
Thanks.
</pre>
</blockquote>
<pre wrap="">
------------------------------------------------------------------------------
The new location for this setup (from os::init_before_ergo) is better
than the earlier proposed change, which had it in
VM_version::early_initialize. The latter is incorrect, because
determine_features (sparc) examines some command-line arguments, and
those aren't parsed until a later stage of Threads::create_vm.
Specifically, determine_features (sparc) is presently looking at two
CLAs: UseV8InstrsOnly (develop) and UseNiagaraInstrs (product). I
suspect UseV8InstrsOnly has served its purpose and could be purged.
But moving the call to determine_features before argument parsing
would have unintentionally ignored UseNiagaraInstrs.
The new location doesn't have this problem. Sorry I missed this in my
earlier review.
------------------------------------------------------------------------------
src/cpu/x86/vm/vm_version_x86.hpp
595 static void vm_init_before_ergo() {};
Trailing semi-colon is not needed.</pre>
</blockquote>
<br>
Fixed.<br>
<br>
<blockquote
cite="mid:DFB0D75F-3696-4640-A1AC-04C434E1FE9E@oracle.com"
type="cite">
<pre wrap="">
------------------------------------------------------------------------------
src/cpu/sparc/vm/vm_version_sparc.cpp
39 guarantee(VM_Version::has_v9(), "only SPARC v9 is supported");
40 assert(_features != VM_Version::unknown_m, "System pre-initialization is not complete.");
I think the assert should be first. The test performed by the
guarantee is really only valid if the pre-initialization has been
performed.</pre>
</blockquote>
<br>
Agreed. Fixed.<br>
<blockquote
cite="mid:DFB0D75F-3696-4640-A1AC-04C434E1FE9E@oracle.com"
type="cite">
<pre wrap="">
------------------------------------------------------------------------------
src/share/vm/runtime/os.cpp
319 VM_Version::vm_init_before_ergo();
This call is in generic code, but only two definitions have been
provided, for sparc and x86. Missing are aarch64, ppc, and zero.</pre>
</blockquote>
<br>
Add vm_init_before_ergo() for those even though JPRT does not<br>
build them? I don't need to be convinced to do it. Just<br>
encouraged. Say "do it" and I'll do it.<br>
<br>
Jon<br>
<br>
<blockquote
cite="mid:DFB0D75F-3696-4640-A1AC-04C434E1FE9E@oracle.com"
type="cite">
<pre wrap="">
------------------------------------------------------------------------------
</pre>
</blockquote>
<br>
</body>
</html>