RFR (M): 8160897: Concurrent mark mark stack memory allocation leaks memory

[Jon Masamitsu]

http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.hpp.frames.html

  175   // Pushes the first "n" elements of "ptr_arr" on the stack.
  176 void par_push_arr(oop* buffer, size_t n); Comment should refer to "buffer" not "ptr_arr"?

  177
178 // Moves up to max elements from the stack into the given buffer. 
Returns
179 // the number of elements pushed, and false if the array has been empty.
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? 673 // 
How many entries will be transferred between global stack and
674 // local queues at once.
675 global_stack_transfer_size = 1024 
http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g1/g1ConcurrentMark.cpp.frames.html 
143 set_empty(); Isn't this taken care of by the constructors?
139 _index(0),

141 _overflow(false),

Does resize() try to reserve new space if the new_capacity is the same
as the current capacity?

146 bool G1CMMarkStack::resize(size_t new_capacity) {

Have you ever cursed the fact that the may data structure (such as
this) double capacity instead of growing more gradually?   Time consider
something different?  Ignore this if it is not the time.

  198   // Double capacity if possible
199 size_t new_capacity = MIN2(old_capacity * 2, MarkStackSizeMax);

That's all.

Jon

On 7/13/2016 1:39 AM, Thomas Schatzl wrote:
> 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:
> http://cr.openjdk.java.net/~tschatzl/8160897/webrev/
>
> Testing:
> vm.gc, jprt, local testing of resizing mechanism
>
> Thanks,
>    Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160713/c104fa2d/attachment.htm>