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