RFR: 8025856 - Fix typos in the GC code
Kirk Pepperdine
kirk at kodewerk.com
Sun Oct 13 14:09:23 UTC 2013
Hi Jesper,
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?
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"?
I'm not a reviewer but that said I didn't see anything else of interest it the checked in bundle.
Regards,
Kirk
On 2013-10-13, at 6:42 AM, Jesper Wilhelmsson <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
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131013/070457c4/attachment.htm>
More information about the hotspot-gc-dev
mailing list