RFR: 8025856 - Fix typos in the GC code
Bengt Rutisson
bengt.rutisson at oracle.com
Sun Oct 13 20:16:10 UTC 2013
Hi Jesper and Kirk,
On 10/13/13 5:39 PM, Jesper Wilhelmsson wrote:
> Kirk Pepperdine skrev 13/10/13 4:09 PM:
>> Hi Jesper,
>
> Hi Kirk,
>
> Thank you for looking at this!
>
>> I know you are a only correcting typos but I found this to be odd in
>> globals.hpp
>>
>> diagnostic(bool, UseNewCode,
>> false, \
>> "Testing Only: Use the new version while testing")
>>
>>
>> Should this flag be set to develop instead of diagnostic?
>
> I'll look into this. I have a few more cleanup patches in the queue so
> I can include it in a future change in case they should be develop. At
> first sight I think it makes sense for these to be develop, but others
> may use the flags in a way that require them to be diagnostic.
l think changing it to a develop flag defeats the purpose of having this
flag at all. The reason UseNewCode exist is that you should be able to
guard some piece of code when you want to try out some new changes.
Normally you want to performance test your changes, so it needs to be
available in product builds.
Bengt
>
>> And
>>
>> --- 287,297 ----
>> // allocation will be concurrent with plain "allocate" calls.
>> virtual bool supports_inline_contig_alloc() const { return false; }
>>
>> // These functions return the addresses of the fields that
>> define the
>> // boundaries of the contiguous allocation area. (These fields
>> should be
>> ! // physical near to one another.)
>>
>>
>> Should that be "physically"?
>
> Yes, I'll fix that.
>
>> I'm not a reviewer but that said I didn't see anything else of
>> interest it the
>> checked in bundle.
>
> Thanks!
> /Jesper
>
>
>>
>> Regards,
>> Kirk
>>
>> On 2013-10-13, at 6:42 AM, Jesper Wilhelmsson
>> <jesper.wilhelmsson at oracle.com
>> <mailto:jesper.wilhelmsson at oracle.com>> wrote:
>>
>>> Hi,
>>>
>>> There were a few more typos in the GC code than the ones I found
>>> before. I
>>> have a new patch that fixes over 200 typos.
>>>
>>> http://cr.openjdk.java.net/~jwilhelm/8025856/webrev/
>>>
>>> There should only be changes in comments. If someone would like to
>>> browse
>>> through these I'd really appreciate it.
>>> /Jesper
>>>
>>>
>>> Jesper Wilhelmsson skrev 3/10/13 1:33 PM:
>>>> Hi,
>>>>
>>>> I've got the necessary reviews for the cleanup part so I will start
>>>> pushing it.
>>>> It has been split into smaller pieces and a couple of new RFEs have
>>>> been created
>>>> for this:
>>>>
>>>> JDK-8025852 - Remove unnecessary setters in collector policy classes
>>>> JDK-8025853 - Remove unnecessary uses of GenerationSizer
>>>> JDK-8025854 - Use "young gen" instead of "eden"
>>>> JDK-8025855 - Simplify GenRemSet code slightly
>>>> JDK-8025856 - Fix typos in the GC code
>>>>
>>>> These are all minor cleanups that happened as a result of the major
>>>> cleanup of
>>>> the collector policy classes. There are a few cleanups in the big
>>>> cleanup patch
>>>> that are not covered by these RFEs. Another RFE may be created to
>>>> cover them.
>>>>
>>>> Thanks,
>>>> /Jesper
>>>>
>>>>
>>>> Jesper Wilhelmsson skrev 30/9/13 10:23 PM:
>>>>> Hi Thomas,
>>>>>
>>>>> Thanks for looking at this!
>>>>>
>>>>> I brought the repository up to date and have a new webrev
>>>>> including your
>>>>> suggestions here:
>>>>>
>>>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev.1/
>>>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.1/
>>>>>
>>>>> Comments inline.
>>>>>
>>>>> Thomas Schatzl skrev 27/9/13 1:29 PM:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, 2013-09-19 at 23:38 +0200, Jesper Wilhelmsson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could I have a couple of reviews of this cleanup/bugfix.
>>>>>>>
>>>>>>> The bug I intended to fix is JDK-8016309 [1] in which the eden
>>>>>>> size could
>>>>>>> end up
>>>>>>> with a zero size due to minimum alignment and a too small young
>>>>>>> gen size.
>>>>>>> The alignment was increased in the fix for JDK-6725714 [2] which
>>>>>>> caused this
>>>>>>> bug.
>>>>>>>
>>>>>>> Fixing this in the current collector policies was quite messy.
>>>>>>>
>>>>>>> Attempts to fix this issue is actually already implemented in
>>>>>>> two different
>>>>>>> places,
>>>>>>> none of them results in a properly sized eden. Since Thomas had
>>>>>>> a patch
>>>>>>> to cleanup the collector policies to fix JDK-7057939 [3], I
>>>>>>> based my
>>>>>>> bugfix on his patch
>>>>>>> instead of fighting with the mess in the current collector
>>>>>>> policies. As Thomas
>>>>>>> wasn't actively working on finishing his cleanup patch I also
>>>>>>> continued that
>>>>>>> work to finish it.
>>>>>>
>>>>>> Thanks a lot for continuing the work!
>>>>>
>>>>> Thanks for starting it ;-)
>>>>>
>>>>>>> This cleanup fixes both bugs (JDK-8016309 and JDK-7057939). To
>>>>>>> extract them to
>>>>>>> different patches would be too much work and result in an
>>>>>>> intermediate state
>>>>>>> between the two patches where the ergonomics wouldn't work
>>>>>>> properly. I have
>>>>>>> however split the webrev into two parts:
>>>>>>>
>>>>>>> The first patch contains cosmetic changes (name changes, being
>>>>>>> consequent in
>>>>>>> using local instance variables vs using getters/setters, fixing
>>>>>>> typos in
>>>>>>> comments etc.).
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev/
>>>>>>>
>>>>>>> The second patch contains the major part of the cleanup work in
>>>>>>> the collector
>>>>>>> policies. The goal has been to make the code easier to follow
>>>>>>> and pull policy
>>>>>>> decisions that had leaked into other code back to the collector
>>>>>>> policy
>>>>>>> classes.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev/
>>>>>>>
>>>>>>> Please note that the default value of NewSize has changed from
>>>>>>> 1M to 2M. Since
>>>>>>> the minimum alignment was increased to 512K, the minimal
>>>>>>> possible young gen
>>>>>>> size
>>>>>>
>>>>>> ... on 64 bit machines, 256k on 32 bit (previously 64k for at least
>>>>>> serial gc I guess)?
>>>>>>
>>>>>> Shouldn't it be possible to still have a MaxHeapSize (or did you
>>>>>> really
>>>>>> mean NewSize here?) of 1M for 32bit (I think this has been the
>>>>>> previous
>>>>>> limit), i.e. each of eden, from, to and old gen a 256k "page"?
>>>>>
>>>>> I did mean NewSize. The MaxNewSize defaults to max_uintx and is
>>>>> later set
>>>>> ergonomically. I didn't want to make the NewSize default value
>>>>> platform
>>>>> dependent, but 2M may be a bit big on 32bit platforms..? I'll
>>>>> check with
>>>>> embedded to see how they feel about this.
>>>>>
>>>>>>> increased to 1.5M. Having a smaller default would be misleading
>>>>>>> as the
>>>>>>> ergonomics would increase it every time.
>>>>>>
>>>>>> I think we should talk to the embedded people about this, but I
>>>>>> think
>>>>>> this 1.5M is a limitation for 64 bit only.
>>>>>
>>>>> Yes.
>>>>>
>>>>>>
>>>>>>> New tests has been added and all existing jtreg arguments tests
>>>>>>> has been
>>>>>>> used to
>>>>>>> verify that heap sizing ergonomics behaves as expected.
>>>>>>>
>>>>>>> Minor note:
>>>>>>> The method GenCollectedHeap::compute_new_generation_sizes was
>>>>>>> unused. In the
>>>>>>> cleanup patch I have commented it out in case anyone left it
>>>>>>> there on purpose.
>>>>>>> I'd prefer to remove it. (Actually the second patch removes the
>>>>>>> method unless
>>>>>>> someone really wants to keep it.)
>>>>>>
>>>>>> Here're the results of a first pass through the change: overall I
>>>>>> think
>>>>>> it's good, but I do need another pass and some more testing. There's
>>>>>> already a few things that could be looked at.
>>>>>>
>>>>>> - In parallelScavengeHeap.cpp:108, it may be good to just pass the
>>>>>> collector policy to the constructor of AdjoiningGenerations
>>>>>> instead of
>>>>>> the bunch of members - but it is fine with me as it is too.
>>>>>
>>>>> I agree. Updated.
>>>>>
>>>>>> - collectorPolicy.cpp:91, "was changed" -> "changed"
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> - in collectorPolicy.cpp:385 there is a huge list of asserts.
>>>>>> Line 329
>>>>>> contains a large part of these: is it possible to make a method like
>>>>>> "validate_ergonomics()" or so to include the ones that are the same
>>>>>> (should be most) and use it in the two places?
>>>>>
>>>>> I cleaned this up and removed the duplicated code. Thanks for
>>>>> pointing this out!
>>>>>
>>>>>> - collectorPolicy.cpp:491: is it possible to put the asserts after
>>>>>> printing the current min/initial/max_gen0 sizes?
>>>>>
>>>>> Fixed in all places.
>>>>>
>>>>>> - could the patch factor out common asserts in the blocks in
>>>>>> collectorPolicy.cpp:491 and :623?
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> - in G1CollectorPolicy::post_heap_initialize() the heap region
>>>>>> size is
>>>>>> updated; note that some earlier change already did this, putting the
>>>>>> update in heapRegion.cpp:177. I prefer to have these sort of updates
>>>>>> collected somewhere (e.g. in post_heap_initialize()), but in any
>>>>>> case we
>>>>>> do not need to do it twice.
>>>>>
>>>>> Since setup_heap_region_size() where the first call was made is
>>>>> called before
>>>>> initialize_flags() I placed the single check in
>>>>> G1CollectorPolicy::initialize_flags where it belongs :-)
>>>>>
>>>>>> - in collectorPolicy.cpp, the expressions of the form
>>>>>> MAX2((uintx)align_size_down(<something>, _min_alignment),
>>>>>> _min_alignment) can be replaced by using the function
>>>>>> restricted_align_down().
>>>>>
>>>>> Fixed. Thanks for pointing that out. restricted_align_down was
>>>>> added by Stefan
>>>>> quite recently so it wasn't around when I did these changes.
>>>>>
>>>>>> - more cleanup: in
>>>>>> GenCollectorPolicy::scale_by_NewRatio_aligned() the
>>>>>> code
>>>>>>
>>>>>> 217 size_t new_gen_size = x > _min_alignment ?
>>>>>> 218 align_size_down(x, _min_alignment) :
>>>>>> _min_alignment;
>>>>>>
>>>>>> is just another form of restricted_align_down
>>>>>
>>>>> True. Fixed.
>>>>>
>>>>>> - in GenCollectorPolicy::initialize_flags(), line
>>>>>>
>>>>>> 293 uintx smallerMaxNewSize = align_size_down(MaxHeapSize -
>>>>>> _min_alignment, _min_alignment);
>>>>>>
>>>>>> you can set smallerMaxNewSize to MaxHeapSize - _min_alignment
>>>>>> directly.
>>>>>> MaxHeapSize must be a multiple of _max_alignment, and
>>>>>> _max_alignment a
>>>>>> multiple of _min_alignment.
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> I think the same argument can be applied to line 304 and 306 and
>>>>>> possibly 312.
>>>>>
>>>>> 304 yes. 306 and 312 no. MaxNewSize and _min_gen0_size are not
>>>>> guarantied to be
>>>>> aligned to _min_alignment here.
>>>>>
>>>>>> - more possible uses of restricted_align_down in collectorPolicy.cpp
>>>>>> line 530 and 536.
>>>>>
>>>>> Fixed.
>>>>>
>>>>>> - I think there is no need to update MaxNewSize again in
>>>>>> GenCollectorPolicy::post_heap_initialize() if it has been taken
>>>>>> care of
>>>>>> earlier already. Maybe just keep the assert.
>>>>>
>>>>> Yes, I missed to remove the code after having run the tests with
>>>>> the assert.
>>>>>
>>>>>> Thanks a lot for looking into this,
>>>>>
>>>>> Thanks for the review!
>>>>> /Jesper
>>>>>
>>>>>> Thomas
>>>>>>
>>>>>>
>>
More information about the hotspot-gc-dev
mailing list