Openjdk hotspot build 14.0-b06 hard coded bug found.

[Jon Masamitsu]

Tony,

Have you signed a Sun contributor agreement?

http://www.sun.com/software/opensource/contributor_agreement.jsp

Jon


Tony Guan wrote On 11/14/08 22:49,:

>Hi Jon,
>
>I've filed a bug report for it. The Review ID is 1390360.
>And I am not sure about the calculated_old_free_size_in_bytes() part,
>so I didn't included your comments. I can look further into the code
>in the coming days.
>
>Thanks!
>
>Tony
>
>On Fri, Nov 14, 2008 at 6:53 PM, Jon Masamitsu <Jon.Masamitsu at sun.com> wrote:
>  
>
>>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.
>>
>>    
>>
>>>
>>>      
>>>
>>>>>          
>>>>>
>>>>        
>>>>
>>    
>>