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

[Thomas Schatzl]

Hi Jon,

  thanks for your quick review.

On Wed, 2016-07-13 at 09:26 -0700, Jon Masamitsu wrote:
> http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g
> 1/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?

Done.

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

 - the local mark stack sizes (128k entries)
 - the global mark stack size (I have been configuring some setups with
1M entries)
 - the push and pop operations take a global lock

I.e. to significantly decrease the overhead.

Note that the follow-up CR to remove the use of the global lock will
remove that constant from there (and practically have the same
elsewhere).

However, that follow-up CR is an enhancement, so subject to an
exemption request.

> 
> http://cr.openjdk.java.net/~tschatzl/8160897/webrev/src/share/vm/gc/g
> 1/g1ConcurrentMark.cpp.frames.html
> 
>  143   set_empty();
> 
> Isn't this taken care of by the constructors?
> 
>  
>  139   _index(0),
>  141   _overflow(false),

I removed the uses in the initialization list. I prefer to reuse
existing code.

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

Yes. However, resize is private, and will never be called with the same
size.

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

There may be some gain to be had there, by using a slower growing
exponential function.

I will create an enhancement request to look into this.

> 
> That's all.

Thanks a lot.

New webrev at
http://cr.openjdk.java.net/~tschatzl/8160897/webrev.1/ (full)
http://cr.openjdk.java.net/~tschatzl/8160897/webrev.0_to_1/ (diff)

Thanks,
  Thomas