<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>