<div dir="ltr"><div>I guess I have no permission to edit code in webreview?</div><div>I wonder if I need to upload a new patch with the renaming? (based on webreview code?)</div><div>Or may you(Thomas) rename these two variables?</div><div><br></div><div>Thanks!</div><div class="gmail_extra"><br><div class="gmail_quote">2016-01-14 17:34 GMT+08:00 Mikael Gerdin <span dir="ltr"><<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<br>
cc:ing Jon<br>
<br>
Jon, can you have a look at the logic in the live_words_in_range optimization performed here? I don't feel that I have enough knowledge about parallel compact to say if it's correct or not.<span class=""><br>
<br>
On 2016-01-13 16:51, Thomas Schatzl wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Tianyang,<br>
<br>
On Sun, 2016-01-10 at 22:16 +0800, ray alex wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi,<br>
By following the conventions, I remove the prefix "get_" and rename<br>
getter/setter to "last_query_*" in this patch version 4 (attached).<br>
Besides, I ran the measurements mentioned before (both for version 3<br>
and recheck this version 4 ), and there was no regressions.<br>
<br>
May I have your reviews on this patch?<br>
<br>
Thanks!<br>
</blockquote>
<br>
  first, sorry for the delay, holidays and such...<br>
<br>
I created a CR for this enhancement at<br>
<a href="https://bugs.openjdk.java.net/browse/JDK-8146987" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8146987</a><br>
<br>
I also made some webrevs for this change that fix some minor issues:<br>
<br>
base patch that includes some compilation fixes:<br>
<a href="http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0/</a><br>
<br>
- recent changes introduced some merge errors in psParallelCompact,<br>
mostly due to Unified logging<br>
- the assert in psParallelCompact.cpp:3137 missed a parameter so the<br>
sources did not compile in any debug mode.<br>
<br>
Some minor style fixes:<br>
<a href="http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~tschatzl/8146987/webrev.1/</a> (full)<br>
<a href="http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0_to_1/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~tschatzl/8146987/webrev.0_to_1/</a> (diff)<br>
</blockquote>
<br>
<br></span>
2151<br>
2152   PSParallelCompact::AdjustPointerClosure closure(cm);<br>
2153   PSParallelCompact::AdjustKlassClosure kclosure(cm);<br>
2154<br>
<br>
Please name these oop_closure and klass_closure.<br>
<br>
Otherwise I think this looks good (modulo getting feedback from Jon)<span class=""><font color="#888888"><br>
<br>
/Mikael</font></span><div class=""><div class="h5"><br>
<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- made a few of the new methods private<br>
- some coding style fixes: indentation, trailing white spaces, copyright dates<br>
- removed the new parMarkBitMap.inline.hpp again because its methods were only called from parMarkBitmap.cpp, fixed includes to parMarkBitmap.cpp.<br>
- prepare some push message, please check<br>
<br>
I did run internal gc testing on the .0 webrev (vm.gc testlist), and did some performance verification (mainly on specjbb201x as this was what was on hand). It does (as expected) improve the perf of full gc, although the improvements are not as high as mentioned in your initial email, around 15% average full gc time (for some rather specific configuration). That is still quite good imo.<br>
<br>
Reproducing the results has been one reason why this reply took a while, it needs a bit of carefully setting up tests to get results that are not drowned in noise due to gc occurring on widely different heap contents, and particularly ergonomics.<br>
Not sure how you managed to do this with something like dacapo. SPECjvm2008 in "realistic" configurations does not do a lot of (if any) full gcs.<br>
<br>
The change looks good to me now, but I need to do some more testing again with the .1 version, to make sure the cleanup did not introduce any regressions anywhere.<br>
<br>
Thanks,<br>
   Thomas<br>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">Lei Tianyang<div><br></div></div>
</div></div>