Openjdk hotspot build 14.0-b06 hard coded bug found.

[Tony Guan]

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.
>
>>
>>
>>
>>>>
>>>>
>>>
>>>
>
>
_______________________________________________
hotspot-gc-use mailing list
hotspot-gc-use at openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/hotspot-gc-use