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

[sangheon]

Hi Thomas,

01 version looks good.

At first, I felt something strange from removing 'line158 _saved_index = 
-1;' from the original g1ConcurrentMark.cpp.
But I understood that there's no problem as we allocate only 1 time.

Thanks,
Sangheon


On 07/13/2016 10:48 AM, Thomas Schatzl wrote:
> 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
>