Openjdk hotspot build 14.0-b06 hard coded bug found.

[Jon Masamitsu]

Inline replies

Tony Guan wrote On 11/14/08 16:00,:

>On Fri, Nov 14, 2008 at 4:58 PM, Jon Masamitsu <Jon.Masamitsu at sun.com> wrote:
>  
>
>>Tony Guan wrote On 11/14/08 14:12,:
>>
>>    
>>
>>>Hi there,
>>>
>>>I think I've found a little bug in the parallel gc codes.  Will
>>>somebody take a look at it?
>>>Firstly, after a full gc, there maybe the need to adjust the boundary
>>>between young and old generation.
>>>and here in psMarkSweep.cpp, we
>>>use:                   heap->resize_old_gen(size_policy->calculated_old_free_size_in_bytes());
>>>
>>>Then this methods will in turn call:
>>>   gens()->adjust_boundary_for_old_gen_needs(desired_free_space);
>>>
>>>in this function, we compare the desired_free_space with the current
>>>free space, and then calls request_old_gen_expansion:
>>>   if (old_gen()->free_in_bytes() < desired_free_space) {
>>>     MutexLocker x(ExpandHeap_lock);
>>>     request_old_gen_expansion(desired_free_space);
>>>but in request_old_gen_expansion the desired_free_space is immediately
>>>treated as expand_in_bytes. And in this implementation, the actual
>>>
>>>
>>>      
>>>
>>Yes, you're right that this looks like a bug.
>>
>>Also something that doesn't look right here is the use of
>>calculated_old_free_size_in_bytes().  It looks like it is adding
>>room for 1 addtional young gen collection.  Perhaps not a
>>bad idea but I don't recall now why it is used instead of the
>>straight promo_size().
>>    
>>
>In my opinion, adding room for 1 additional young gen collection is
>OK, because in adjustable parallel gc, by doing this, we can reduce
>the chance of doing full gc for next time. And that's why it's called
>desired_free_space.  (just a guess)
>  
>

I think it's odd because "desire_free_size" should have any such
additional space built into it.  "desired_free_size" is set in a
method that purports to calculate the correct amount of free
space for the old generation yet the use of
calculated_old_free_size_in_bytes() bumps that up just a bit.
Probably doesn't hurt but it's a policy decision that should
be embedded in the calculation of "desired_free_size".

>
>  
>
>>    
>>
>>>change in bytes is computed like this:
>>>size_t change_in_bytes = MIN3(young_gen_available,
>>>                               old_gen_available,
>>>                               align_size_up_(expand_in_bytes, alignment));
>>>and then:
>>>virtual_spaces()->adjust_boundary_up(change_in_bytes)
>>>
>>>So in the end, we have old_gen()->free_in_bytes()+=desired_free_space
>>>and the final "desired_free_space" is more than needed.
>>>
>>>And after the boundary is moved up (too high), the
>>>_old_gen->resize(desired_free_space)
>>>
>>>will compute the new space size as:
>>>size_t new_size = used_in_bytes() + desired_free_space;
>>>
>>>Thus it will shrink the old gen space smaller. So we have part the
>>>space sacrificed by the young generation out of the old gen usage. And
>>>the size of this wasted memory is (desired_free_space- prevoiusly free
>>>space of old_gen).
>>>
>>>
>>>      
>>>
>>Yup, we've moved the boundary farther than we
>>really wanted to.
>>
>>Are you able to file a bug report for this?  Did you want
>>to fix it?
>>    
>>
>
>Is there any format should I follow to report the bug?
>  
>
In the description section if you just included the info you
put in your mail, that would be fine.  You can
even cut-and-paste you mail into it.

>In fact it's simple to fix it. just change
>in
>void AdjoiningGenerations::adjust_boundary_for_old_gen_needs()
>the following change will solve it:
>
>-request_old_gen_expansion(desired_free_space);
>+request_old_gen_expansion(desired_free_space-old_gen()->free_in_bytes());
>
>  
>

Yes, it's a straight forward change and so is a nice one if you
want to try you hand at a contribution.

>
>  
>
>>>      
>>>
>>    
>>

_______________________________________________
hotspot-gc-use mailing list
hotspot-gc-use at openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/hotspot-gc-use