<div dir="ltr"><div>Christian, Vladimir thank you for your reviews!</div><div><br></div><div>Please find the new webrev at: <a href="http://cr.openjdk.java.net/~anoll/8015774/webrev.01/">http://cr.openjdk.java.net/~anoll/8015774/webrev.01/</a></div>
<div><br></div><div>See comments for both reviews inline:</div><div class="gmail_extra">

<br></div><div class="gmail_extra"><br><div class="gmail_quote">2013/10/8 Vladimir Kozlov <span dir="ltr">&lt;<a href="mailto:vladimir.kozlov@oracle.com" target="_blank">vladimir.kozlov@oracle.com</a>&gt;</span><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">ProfiledCodeHeapSize value is missleading because part of it will be cut for non_method section:<br>



<br>
profile_size = ProfiledCodeHeapSize - non_method_size;<br>
<br>
And you hided default non_method_size (8Mb) inside the code:<br>
<br>
+  size_t non_method_size = ReservedCodeSpace::allocation_<u></u>align_size_up(MIN2(<u></u>ReservedCodeCacheSize, 8*M));<br>
<br>
It is not good. If in future we add more stubs, for example, 8Mb could be not enough and it will be hard to find this place. May be<br></blockquote><div> </div><div>I agree, this part is still a little bit unclear. This is partly due to the fact that I&#39;m still not sure how to set the size of the non method heap. On my machine around 3.5Mb are sufficient for all benchmarks (debug/product). The jprt tests seem to need more, that&#39;s why it is temporarily set to 8Mb. I have to evaluate that in more detail. Probably we can set it depending on build/runtime settings and operating system.</div>


<div><br></div><div>One option would be to create a new heap for all blobs needed by the VM to startup. This heap size can be set to CodeCacheMinimumUseSpace. All non method blobs created by the running java application are then put into the non method heap.  </div>


<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
NonProfiledCodeHeapSize + ProfiledCodeHeapSize == ReservedCodeCacheSize + 8Mb<br>
<br>
So the size could be adjusted by setting 2 flags.<br></blockquote><div> </div><div> Do you mean that the 8Mb for the non method heap should be subtracted half from the NonProfiledCodeHeapSize and half from ProfiledCodeHeapSize?</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Why you need all heaps for Client VM (only C1 is available)?<br>
<br>
+bool CodeCache::heap_available(<u></u>CodeBlobType bt) {<br>
+  if (TieredCompilation || bt == btNonMethod) {<br>
+    // Use all heaps for TieredCompilation<br>
+    return true;<br>
+  } else {<br>
+#ifdef COMPILER2<br>
+    // We only need the heap for C2<br>
+    return (bt == btMethodNoProfile);<br>
+#else<br>
+    // We need both heaps<br>
+    return true;<br>
+#endif<br>
+  }<br>
 }<br></blockquote><div> </div><div>If we have a client build with only the C1 compiler available we still need the non profiled code heap, because we still have the compilation levels CompLevel_none (native methods) and CompLevel_simple (C1 compiled without profiling) that belong in this heap. </div>


<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
report_codemem_full(). When I discussed this feature with Igor, he told me that if you failed allocate in one heap you can allocate in an other heap. Or you can move boundary. The longer you run &quot;stable&quot; application the less ProfiledCodeHeap will be used (could be changed later when we add code to fall back to profiled code during nmethod deoptimization). It would be nice to redirect its space for NonProfiledCodeHeap. For this purpose layout of heaps should be reversed.<br>




It would be strange to see free space in whole codecache but not able to allocate.<br></blockquote><div><br></div><div>I&#39;m not sure if I understood your point correctly. At the moment we delay compilation for the corresponding compilation level if a heap is full. In practice (at least for the benchmarks I run) the second heap then fills up really fast. Your suggestion to allocate CodeBlobs in another heap somehow &quot;breaks&quot; the design, because the client of the CodeCache could then encounter non profiled, e.g. C2 compiled, methods while iterating over the profiled heap. This is not really a problem at the moment, since no code relies on the fact that the non profiled heap _only_ contains C2 methods (as long as it contains only _methods_). </div>


<div>However, I think that the second suggestion to move the boundary between the heaps is better. One could then increase the non profiled heap and decrease the profiled heap size if the first is full. Over time the non profiled heap would probably grow, whereas the profiled heap would shrink.</div>


<div><br></div><div>Because I suspect this to be another big change, I would however suggest to implement this behavior with a second patch and first finish the current changes. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">



Next message is incorrect now since you need heap&#39;s specific flag:<br>
<br>
+    warning(&quot;Try increasing the code cache size using -XX:ReservedCodeCacheSize=&quot;);<br></blockquote><div><br></div><div>It is not incorrect as increasing the ReservedCodeCacheSize also increases the non profiled and the profiled heap sizes (if no explicit value is set by the user). One option would be to print &quot;Try increasing the code cache size using -XX:ReservedCodeCacheSize= or -XX:NonProfiledCodeHeapSize&quot; (or &quot;-XX:ProfiledCodeHeapSize&quot;, depending on the heap that is full). </div>


<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
thanks,<br>
Vladimir</blockquote><div> </div>2013/10/4 Christian Thalinger <span dir="ltr">&lt;<a href="mailto:christian.thalinger@oracle.com" target="_blank">christian.thalinger@oracle.com</a>&gt;</span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


<div style="word-wrap:break-word"><div>Some general comments:</div><div><br></div><div>Don&#39;t use T1, T2, … to name the tiers because SPARC processors are named like this.  It&#39;s confusing.  Use tier 1, tier 2, … instead.  The compilation policies actually talk about tier levels...</div>


</div></blockquote><div><br></div><div>I changed the names of the code heaps and comments accordingly. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


<div style="word-wrap:break-word"><div></div><div>I was hoping that we can have a more loose definition of the code heaps.  So we can easily add more.  What this patch does is a good start but all the hardcoded values and fields need to go at a later point in time, in my opinion.</div>


</div></blockquote><div><br></div><div><div>To improve the adding of new heaps I completely removed the direct dependencies on code heaps from the serviceability agent, dtrace and the pstack library, especially the fields _heap_non_method, _heap_non_profiled and _heap_profiled fields in the code cache. The GrowableArray&lt;CodeHeap*&gt;* is now the only field used to reference the code heaps. This allows adding new heaps to the cache without the need to adapt references. An exception is the dtrace ustack helper script (jhelper.d). Because the D language does not allow loops, one has to define a probe for each code heap. For now, the script supports up to 5 code heaps, but more can be added by simply adding new probes to the script (comment in line 183 in jhelper.d describes this). </div>


<div><br></div><div>To summarize, if I want to add a new code heap to the cache, all I have to do is to </div><div>- define a new CodeBlobType (and add it to IsMethodPredicate if it is a method type)</div><div>- create the heap in CodeCache::initialize_heaps() and define default sizes</div>


<div>[- define heap availability in CodeCache::heap_available]</div><div>[- define the compilation level -&gt; code blob type translation in CodeCache::get_code_blob_type]</div><div><br></div><div>The last two steps are optional. CodeBlobs created with the new CodeBlobType are then allocated in the corresponding heap. </div>


<div><br></div><div>I&#39;m not completely sure what you mean with &quot;hardcoded&quot; values. Do you mean the default values for NonProfiledCodeHeapSize and ProfiledCodeHeapSize? Of course we could compute them implicitly as 1/3 and 2/3 of the ReservedCodeCacheSize (as it is now done if the user does not provide a value), without defining them as VM parameter, but otherwise we have to define default values for them.</div>


</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>

</div>
<div>src/share/vm/code/codeBlob.hpp:</div><div><pre><span style="color:blue">+// CodeBlob Types</span>
<span style="color:blue">+// Used in the CodeCache to assign CodeBlobs to different CodeHeaps</span>
<span style="color:blue">+enum CodeBlobType {</span>
<span style="color:blue">+  btMethodNoProfile   = 0,    // T1 and T4 methods (including native methods)</span>
<span style="color:blue">+  btMethodProfile     = 1,    // T2 and T3 methods with profile information</span>
<span style="color:blue">+  btNonMethod         = 2     // Non-methods like Buffers, Adapters and Runtime Stubs</span>
<span style="color:blue">+};</span></pre></div><div>The bt prefix is already overloaded (e.g. BasicType).  Generally speaking, I never liked the fact that enums don&#39;t have their name as scope.  Maybe we can agree on doing something like this:</div>


<div><br></div><div><div>struct CodeBlobType {</div><div>  enum Enum {</div><div>    NonMethod = 2</div><div>  };</div><div>};</div><div><br></div><div>void foo() {</div><div>  int a = CodeBlobType::NonMethod;</div><div>

  int b = (CodeBlobType::Enum) 1;</div>
<div>}</div></div><div><br></div><div>Or we move to C++11 :-)</div><div><br></div><div><a href="http://www.stroustrup.com/C++11FAQ.html#enum" target="_blank">http://www.stroustrup.com/C++11FAQ.html#enum</a></div></div></blockquote>


<div><br></div><div>Yes, it is probably more clear to include the scope. I followed your suggestion and changed it to a struct with an anonymous enum. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">


<div style="word-wrap:break-word"><div>src/share/vm/code/codeCache.cpp:</div><div><pre><span style="color:blue">+#define FOR_ALL_HEAPS(index) for (int index = 0; index &lt; _heaps-&gt;length(); ++index)</span></pre></div>


<div>I&#39;m one of the people who started to hate macros (for various reasons).  Although I see the point in defining these I&#39;d rather go without them.  Is there a way we can use STL vectors with our allocators?  Or we could write our own iterator class which iterates either over GrowableArrays in general or the code heaps in particular.</div>


</div></blockquote><div><br></div><div>I perfectly agree with you that macros should be used only if necessary and to improve readability in special cases. I checked the option of using STL vectors but since the STL is not used in the code yet and I&#39;m not sure if they can be used with the hotspot allocators, I think it is better to work with own iterator classes. I therefore defined the custom iterator classes GrowableArrayIterator and GrowableArrayFilterIterator. They implement the STL input iterator interface without actually inheriting from it (since #include &lt;iterator&gt; seems not to work with solaris). The filter iterator is used to iterate only over those code heaps satisfying a given predicate, for example those heaps containing nmethods. This makes the code a lot more readable and reduces the number of macros. I think the remaining four macros, namely FOR_ALL_HEAPS and FOR_ALL_METHOD_HEAPS to iterate over the heaps and FOR_ALL_BLOBS and FOR_ALL_ALIVE_BLOBS to iterate over the blobs are justified, because they are simple (only declaring the iterators) and make the code more readable.<br>


</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>

</div>
src/share/vm/memory/heap.hpp:<div><pre><span style="color:blue">+  const char* getName() const   { return _name; }</span>
</pre><div>This should either be get_name() or name().</div></div></div></blockquote><div> </div><div>I renamed it to name(). <br></div><div><br></div><div><div>Thanks!</div><div><br></div><div>Best regards</div></div><div>


Tobias</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
<br>
On 10/2/13 7:06 AM, Tobias Hartmann wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi,<br>
<br>
please review the following change.<br>
<br>
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8015774" target="_blank">https://bugs.openjdk.java.net/<u></u>browse/JDK-8015774</a><br>
webrev: <a href="http://cr.openjdk.java.net/~anoll/8015774/webrev.00/" target="_blank">http://cr.openjdk.java.net/~<u></u>anoll/8015774/webrev.00/</a><br>
<br>
This change implements support for multiple code heaps in the code<br>
cache. The interface of the code cache was changed accordingly and<br>
references from other components of the VM were adapted. This includes<br>
the indirect references from:<br>
- the Serviceability Agent: vmStructs and the Java code cache interface<br>
(sun.jvm.hotspot.code.<u></u>CodeCache)<br>
- the dtrace ustack helper script (jhelper.d)<br>
- the pstack support library libjvm_db.c<br>
<br>
Currently the code cache contains the following three code heaps each of<br>
which contains CodeBlobs of a specific type:<br>
- Non-Profiled methods: nmethods that are not profiled, i.e., those<br>
compiled at tier 1 or 4 and native methods<br>
- Profiled methods: nmethods that are profiled, i.e., those compiled at<br>
tier 2 or 3<br>
- Non-methods: Non-methods like Buffers, Adapters and Runtime Stubs<br>
<br>
By default, 2/3 of the ReservedCodeCacheSize is used for the<br>
non-profiled heap and 1/3 is used for the profiled heap. Experiments<br>
with a small code cache size have shown that this configuration performs<br>
best (see [1]). Sizes can be configured using the<br>
NonProfiledCodeHeapSize and ProfiledCodeHeapSize parameters. By now the<br>
non-method heap has a fixed size of 8mb. More tests have to be performed<br>
to determine reasonable default values for different build and runtime<br>
configurations. It would be also possible to add an additional heap for<br>
the CodeBlobs needed during VM startup (adapters, stubs, interpreter,<br>
...) and use the non-method heap for non-method blobs allocated during<br>
runtime.<br>
<br>
The main benefit of this allocation is that functions operating on<br>
nmethods can now be optimized to only iterate over CodeBlobs on the<br>
nmethod heaps, avoiding a full scan of the code cache including the<br>
non-methods. Performance evaluation shows that this results in a great<br>
reduction of the sweeping time. Further it is now possible to extend the<br>
sweeper to selectively sweep nmethods of different compilation levels.<br>
Because it is more expensive to recompile a highly optimized method, it<br>
makes sense to first sweep those compiled at a lower level. Possible<br>
future optimizations may also include multithreaded sweeping and<br>
separation of nmethod code and metadata.<br>
<br>
The code was tested using Nashorn + Octane, DaCapo, SPECJvm2008 and jprt.<br>
<br>
Benchmark results:<br>
- [2] shows the score of the old and new version running Nashorn with<br>
the Octane benchmark at different code cache sizes (10 runs each, sizes<br>
&lt; 32mb crash with the old version). An performance improvement of around<br>
15% is reached.<br>
- [3] shows the sweep time of the NMethodSweeper during runs of the<br>
Octane benchmark with different code cache sizes (10 runs each). The<br>
time is greatly reduced with the new version.<br>
- [4] shows the time ratio (TOld / TNew - 1) of the DaCapo benchmarks<br>
with a code cache size of 64mb (30 runs with 20 warmups each). With the<br>
time differences being smaller than the confidence intervals it is not<br>
possible determine a difference between the two versions.<br>
- Multiple runs of the SPECJvm2008 benchmark with different code cache<br>
sizes did not show a difference between the two versions. This is<br>
probably due to the fact that not much code is compiled at runtime<br>
compared to Nashorn with the Octane benchmark.<br>
<br>
The error bars at each data point show the 95% confidence interval.<br>
<br>
[1] <a href="https://bugs.openjdk.java.net/secure/attachment/16372/OctaneRatio.png" target="_blank">https://bugs.openjdk.java.net/<u></u>secure/attachment/16372/<u></u>OctaneRatio.png</a><br>
[2]<br>
<a href="https://bugs.openjdk.java.net/secure/attachment/16374/OctaneCCSizes_new.png" target="_blank">https://bugs.openjdk.java.net/<u></u>secure/attachment/16374/<u></u>OctaneCCSizes_new.png</a><br>
[3]<br>
<a href="https://bugs.openjdk.java.net/secure/attachment/16373/OctaneSweepTime_new.png" target="_blank">https://bugs.openjdk.java.net/<u></u>secure/attachment/16373/<u></u>OctaneSweepTime_new.png</a><br>
[4]<br>
<a href="https://bugs.openjdk.java.net/secure/attachment/16371/DaCapoCCSizes_64.png" target="_blank">https://bugs.openjdk.java.net/<u></u>secure/attachment/16371/<u></u>DaCapoCCSizes_64.png</a><br>
<br>
Thanks and best regards,<br>
<br>
Tobias<br>
</blockquote>
</div></div></blockquote></div><br></div></div>