Hi,<br><br>Updated patch is here<br><a href="http://cr.openjdk.java.net/%7Ebrutisso/7068625/webrev.01/" target="_blank">http://cr.openjdk.java.net/~brutisso/7068625/webrev.01/</a><br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
From: John Coomes <<a href="mailto:John.Coomes@oracle.com">John.Coomes@oracle.com</a>><br>
Subject: Re: Fwd: (resend) Request for review (S): 7068625 Testing 8<br>
bytes of card table entries at a time speeds up card-scanning<br>
To: Bengt Rutisson <<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>><br>
Cc: "<a href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>"<br>
<<a href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a>><br>
Message-ID: <<a href="mailto:20300.16537.700270.474085@oracle.com">20300.16537.700270.474085@oracle.com</a>><br>
Content-Type: text/plain; charset=us-ascii<br>
<br>
Bengt Rutisson (<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>) wrote:<br>
><br>
> Hi all,<br>
><br>
> Just pinging this review request. Does anybody have some time to look at<br>
> it? It is a fairly small and straight forward change...<br>
<br>
Semantically looks ok. Some style issues, though.<br>
<br>
cardTableRS.cpp:<br>
---------------<br>
<br>
I'd rather see the loop on 207 rewritten as:<br>
<br>
207 while (cur_row >= limit &&<br>
208 *((intptr_t*)cur_row) == CardTableRS::clean_card_row()) {<br>
209 cur_row -= BytesPerWord;<br>
210 }<br></blockquote><div><br>done<br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
as it's shorter and eliminates the 'break'.<br>
<br>
There are also whitespace problems (inconsistent indentation, 4 spaces<br>
instead of 2, no space after while, etc.).<br>
<br>
The comment that starts with 'Reset the dirty window' is now in only<br>
one of the places that does the reset. And this comment:<br>
<br>
// Note that "cur_entry" leads "start_of_non_clean" in<br>
// its leftward excursion after this point<br>
// in the loop and, when we hit the left end of "mr",<br>
// will point off of the left end of the card-table<br>
// for "mr".<br>
}<br>
<br>
is in an odd position (no code follows it) and the 'after' should be<br>
changed to 'at'.<br>
<br>
But instead of changing the comments, the new behavior can be achieved<br>
with a single new hunk of code in do_MemRegion that keeps the comments<br>
relevant:<br>
<br>
--- <a href="http://cardTableRS.cpp.org" target="_blank">cardTableRS.cpp.org</a><br>
+++ cardTableRS.cpp<br>
@@ -17,10 +17,22 @@<br>
// "dirty" range accumulated so far.<br>
if (start_of_non_clean < end_of_non_clean) {<br>
const MemRegion mrd(start_of_non_clean, end_of_non_clean);<br>
_dirty_card_closure->do_MemRegion(mrd);<br>
}<br>
+<br>
+ // fast forward through potential continuous range of clean cards<br>
+ if (is_word_aligned(cur_entry)) {<br>
+ jbyte* cur_row = cur_entry - BytesPerWord;<br>
+ while (cur_row >= limit &&<br>
+ *((intptr_t*)cur_row) == CardTableRS::clean_card_row()) {<br>
+ cur_row -= BytesPerWord;<br>
+ }<br>
+ cur_entry = cur_row + BytesPerWord;<br>
+ cur_hw = _ct->addr_for(cur_row + BytesPerWord);<br>
+ }<br>
+<br>
// Reset the dirty window, while continuing to look<br>
// for the next dirty card that will start a<br>
// new dirty window.<br>
end_of_non_clean = cur_hw;<br>
start_of_non_clean = cur_hw;<br>
<br>
I think it's easier to follow.<br>
<br></blockquote><div><br>agreed<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
cardTableRS.hpp:<br>
---------------<br>
<br>
48 // making this constants into inline methods kills performance for some reason<br>
49 static intptr_t clean_card_row() {<br>
50 return CardTableModRefBS::clean_card_row;<br>
51 }<br>
<br>
The comment seems to warn against the very thing the code is doing.<br>
If the comment is accurate, it's fine to eliminate the method and use<br>
the enum directly.<br></blockquote><div><br>comment was included in patch by accident, removed<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
53 static bool card_is_dirty_wrt_gen_iter(jbyte cv) {<br>
54 return CardTableModRefBS::card_is_dirty_wrt_gen_iter(cv);<br>
<br>
This whitespace change looks spurios.<br></blockquote><div><br>true<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-John<br>
<br>
> -------- Original Message --------<br>
> Subject: Request for review (S): 7068625 Testing 8 bytes of card table<br>
> entries at a time speeds up card-scanning<br>
> Date: Tue, 21 Feb 2012 12:03:50 +0400<br>
> From: Alexey Ragozin <<a href="mailto:alexey.ragozin@gmail.com">alexey.ragozin@gmail.com</a>><br>
> To: <a href="mailto:hotspot-gc-dev@openjdk.java.net">hotspot-gc-dev@openjdk.java.net</a><br>
> CC: Bengt Rutisson <<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>><br>
><br>
><br>
><br>
> Hi,<br>
><br>
> I would like few volunteers to review changes for<br>
> <a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068625" target="_blank">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068625</a><br>
> WebRev: <a href="http://cr.openjdk.java.net/%7Ebrutisso/7068625/webrev.00/" target="_blank">http://cr.openjdk.java.net/~brutisso/7068625/webrev.00/</a><br>
> <<a href="http://cr.openjdk.java.net/%7Ebrutisso/7068625/webrev.00/" target="_blank">http://cr.openjdk.java.net/%7Ebrutisso/7068625/webrev.00/</a>><br>
><br>
> Change summary<br>
> For large heaps (I was focusing on 8GiB and above) it is common to have<br>
> long continuous ranges of clean cards.<br>
> Patch is introducing a short path for skipping ranges of clean cards<br>
> using word aligned memory access instead of byte aligned.<br>
><br>
> Patch affects serial and CMS collectors. For CMS collector stride size<br>
> should be increase to see any performance gains (I was using<br>
> -XX:+UnlockDiagnosticVMOptions<br>
> -XX:ParGCCardsPerStrideChunk=4096)<br>
><br>
> For testing I was mainly using synthetic benchmark randomly modifying<br>
> hash tables in heap, thus uniformly touching cards across heaps.<br>
> Average duration of young GC pause were used as KPI.<br>
> More details about testing can be found at<br>
> <a href="http://blog.ragozin.info/2011/07/openjdk-patch-cutting-down-gc-pause.html" target="_blank">http://blog.ragozin.info/2011/07/openjdk-patch-cutting-down-gc-pause.html</a><br>
> Though article is referring jdk6, my resent tests with trunk jdk7 show<br>
> no difference.<br>
> I was also tested patch with real application (Oracle Coherence storage<br>
> node).<br>
> With 16GiB of heap and CMS/ParNew GC, enabling patch have shortened GC<br>
> pauses roughly in 2 times.<br>
><br>
> Source code of benchmark used in test are available at<br>
> <a href="https://gridkit.googlecode.com/svn/branches/aragozin-sandbox/young-gc-bench" target="_blank">https://gridkit.googlecode.com/svn/branches/aragozin-sandbox/young-gc-bench</a><br>
> Main class YoungGCPauseBenchmark<br>
><br>
> Regards,<br>
> Alexey<br>
><br></blockquote></div><br>