<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.hpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.hpp.frames.html</a><br>
<br>
<pre> 175 // Pushes the first "n" elements of "ptr_arr" on the stack.
<span class="changed">176 void par_push_arr(oop* buffer, size_t n);
</span>Comment should refer to "buffer" not "ptr_arr"?
<span class="changed">177 </span>
<span class="changed"> 178 // Moves up to max elements from the stack into the given buffer. Returns</span>
<span class="changed"> 179 // the number of elements pushed, and false if the array has been empty.</span>
<span class="changed"> 180 bool par_pop_arr(oop* buffer, size_t max, size_t* n);
Do you want to comment on return value?
What motivated this increase to 1024?
</span><span class="changed"> 673 // How many entries will be transferred between global stack and</span>
<span class="changed"> 674 // local queues at once.</span>
<span class="changed"> 675 global_stack_transfer_size = 1024
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.cpp.frames.html</a>
</span><span class="changed"> 143 set_empty();
Isn't this taken care of by the constructors?
</span><span class="changed"> </span>
<span class="changed"> 139 _index(0),</span></pre>
<pre><span class="changed"> 141 _overflow(false),
</span><span class="changed"></span>
</pre>
Does resize() try to reserve new space if the new_capacity is the
same<br>
as the current capacity?<br>
<pre><span class="changed"></span><span class="changed"> 146 bool G1CMMarkStack::resize(size_t new_capacity) {
</span><span class="changed"></span>
</pre>
Have you ever cursed the fact that the may data structure (such as<br>
this) double capacity instead of growing more gradually? Time
consider<br>
something different? Ignore this if it is not the time.<br>
<pre> 198 // Double capacity if possible
<span class="changed"> 199 size_t new_capacity = MIN2(old_capacity * 2, MarkStackSizeMax);
</span><span class="changed"></span>
</pre>
That's all.<br>
<br>
Jon<br>
<pre><span class="changed"></span></pre>
<pre><span class="changed"></span></pre>
<div class="moz-cite-prefix">On 7/13/2016 1:39 AM, Thomas Schatzl
wrote:<br>
</div>
<blockquote cite="mid:1468399172.2318.17.camel@oracle.com"
type="cite">
<pre wrap="">Hi all,
can I have reviews for the following bugfix that makes global mark
stack memory management a little bit more sane:
- fixed a typo in some comment which (imho) reversed its meaning
- during resize of the mark stack,
- g1 does not unreserve that memory, it only tries and fails to
uncommit the memory (calling the wrong method).
- g1 does not assign the new memory the "mtGC" tag.
- if g1 fails reserving that new memory, we exit the VM. This is in
contrast to the original idea of just trying to continue with the
existing mark stack.
- during initial allocation of the mark stack, we issue a warning and
exit the constructor right away, leaving the class half-uninitialized.
This has been changed to a vm_exit_at_initialization() error.
- removed code duplication
- fixed indices/capacity types from int to size_t to avoid casting
around (subsuming JDK-8079081). I do not think it makes sense to mostly
rewrite G1CMMarkStack and then still inappropriately use ints.
- tried to fix up log messages
The change does not address:
- the (weird) policy when to actually increase the mark stack (is JDK-
8065402).
- recreate mappings during expansion, but could reserve all space up-
front and (un-commit) as necessary (is JDK-8151996)
- mark stack contention (follow-up JDK-8159422)
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8160897/webrev/">http://cr.openjdk.java.net/~tschatzl/8160897/webrev/</a>
Testing:
vm.gc, jprt, local testing of resizing mechanism
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>