<html><body><p><font size="2">Hi Martin,</font><br><br><font size="2">Thanks a lot for your review. Sure, we need an OK from a CMS expert. Following is the new webrev:</font><br><a href="http://cr.openjdk.java.net/~mhorie/8205908/webrev.01/"><font size="2">http://cr.openjdk.java.net/~mhorie/8205908/webrev.01/</font></a><br><br>>Seems like a user of the forwardee needs to rely on memory_order_consume in the current implementation. I guess it will be appreciated that you’re fixing this.<br>Thank you for pointing out this issue in the original implementation. I newly inserted a release at "<font size="2">2.4. Set new_obj as forwardee [L1142]</font>".<br><br><font size="2">Improvement of critical-jOPS in SPECjbb2015 was 10%, which is still a big number.</font><br><br><br><font size="2">Best regards,</font><br><font size="2">--</font><br><font size="2">Michihiro,</font><br><font size="2">IBM Research - Tokyo</font><br><br><img width="16" height="16" src="cid:1__=8FBB082CDFB955D68f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for "Doerr, Martin" ---2018/07/02 16:56:03---Hi Michihiro, thanks for addressing this issue."><font size="2" color="#424282">"Doerr, Martin" ---2018/07/02 16:56:03---Hi Michihiro, thanks for addressing this issue.</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">"Doerr, Martin" <martin.doerr@sap.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">Michihiro Horie <HORIE@jp.ibm.com>, "hotspot-gc-dev@openjdk.java.net" <hotspot-gc-dev@openjdk.java.net></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">Kim Barrett <kim.barrett@oracle.com>, Gustavo Romero <gromero@linux.vnet.ibm.com></font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">2018/07/02 16:56</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">RE: 8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space</font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br>Hi Michihiro,<br> <br>thanks for addressing this issue.<br> <br>The change looks good to me. I only have a comment on the coding style (oop.inline.hpp): “if ()” should be followed by braces “{ … }”.<br> <br>Seems like a user of the forwardee needs to rely on memory_order_consume in the current implementation. I guess it will be appreciated that you’re fixing this.<br> <br>Please note that SAP still supports CMS in the commercial VM so this change is still relevant and we’d like to push it to jdk11 if possible.<br> <br>But we definitely need an OK from a CMS expert (which I’m not).<br> <br>Best regards,<br>Martin<br> <br> <br><b>From:</b> Michihiro Horie [<a href="mailto:HORIE@jp.ibm.com">mailto:HORIE@jp.ibm.com</a>] <b><br>Sent:</b> Mittwoch, 27. Juni 2018 02:23<b><br>To:</b> hotspot-gc-dev@openjdk.java.net<b><br>Cc:</b> Doerr, Martin <martin.doerr@sap.com>; Kim Barrett <kim.barrett@oracle.com>; Gustavo Romero <gromero@linux.vnet.ibm.com><b><br>Subject:</b> RFR: 8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space<br>
<p><font size="2">Dear all,</font><br><font size="2"><br>Would you please review the following change?<br>Bug: </font><a href="https://bugs.openjdk.java.net/browse/JDK-8205908"><u><font size="2" color="#0000FF">https://bugs.openjdk.java.net/browse/JDK-8205908</font></u></a><font size="2"><br>Webrev: </font><a href="http://cr.openjdk.java.net/~mhorie/8205908/webrev.00/"><u><font size="2" color="#0000FF">http://cr.openjdk.java.net/~mhorie/8205908/webrev.00/</font></u></a><br><br><font size="2"><br>[Current implementation]<br>ParNewGeneration::copy_to_survivor_space tries to move live objects to a different location. There are two patterns on how to copy an object depending on whether there is space to allocate new_obj in to-space or not. If a thread cannot find space to allocate new_obj in to-space, the thread first executes the CAS with a dummy forwarding pointer "ClaimedForwardPtr", which is a sentinel to mark an object as claimed. After succeeding in the CAS, a thread can copy the new_obj in the old space. Here, suppose thread A succeeds in the CAS, while thread B fails in the CAS. When thread A finishes the copy, it replaces the dummy forwarding pointer with a real forwarding pointer. After thread B fails in the CAS, thread B returns the forwardee after waiting for the copy of the forwardee is completed. This is observable by checking the dummy forwarding pointer is replaced with a real forwarding pointer by thread A. In contrast, if a thread can find space to allocate new_obj in to-space, the thread first copies the new_obj and then executes the CAS with the new_obj. If a thread fails in the CAS, it deallocates the copied new_obj and returns the forwardee.</font><br><font size="2"><br>Procedure of ParNewGeneration::copy_to_survivor_space : ([L****] represents the line number in src/hotspot/share/gc/cms/parNewGeneration.cpp) <br>1. Try to each allocate space for new_obj in to-space [L.1110]<br>2. If fail in the allocation in to-space [L1117] <br>2.1. Execute the CAS with the dummy forwarding pointer [L1122] ——— (A)<br>2.2. If fail in the CAS, return the forwardee via real_forwardee() [L1123]<br>2.3. If succeed in the CAS [L1128] <br>2.3.1. If promotion is allowed, copy new_obj in the old area [L1129]<br>2.3.2. If promotion is not allowed, forward to obj itself [L1133]<br>2.4. Set new_obj as forwardee [L1142]<br>3. If succeed in the allocation in to-space [L1144] <br>3.1. Copy new_obj [L1146]<br>3.2. Execute the CAS with new_obj [L1148] ——— (B)<br>4. Dereference the new_obj for logging. Each new_obj copied by each thread at step 3.1 is used instead of forwardee() [L1159]<br>5. If succeed in either CAS (A) or CAS (B), return new_obj [L1163]<br>6. If fail in CAS (B), get the forwardee via real_forwardee(). Unallocate new_obj in to-space [L1193]<br>7. Return forwardee [L1203]</font><br><font size="2"><br>For reference, real_forwardee() is as shown below:<br>oop ParNewGeneration::real_forwardee(oop obj) {<br>oop forward_ptr = obj->forwardee();<br>if (forward_ptr != ClaimedForwardPtr) {<br>return forward_ptr;<br>} else {<br>// manually inlined for readability.<br>oop forward_ptr = obj->forwardee();<br>while (forward_ptr == ClaimedForwardPtr) {<br>waste_some_time();<br>forward_ptr = obj->forwardee();<br>}<br>return forward_ptr;<br>}<br>}</font><br><font size="2"><br>Regarding the CAS (A),<br>There is no copy before the CAS.<br>Dereferencing the forwardee must be allowed after obtaining the forwardee.</font><br><font size="2"><br>Regarding the CAS (B),<br>There is a copy before the CAS.<br>Dereferencing the forwardee must be allowed after obtaining the forwardee.</font><br><br><font size="2"><br>[Observation on the current implementation]<br>No fence is necessary before and after the CAS (A).<br>Release barrier is necessary before the CAS (B).<br>The forwardee_acquire() must be used instead of forwardee() in real_forwardee().</font><br><br><font size="2"><br>[Performance measurement]<br>The critical-jOPS of SPECjbb2015 improved by 12% with this change.</font><br><br><font size="2"><br>Best regards,<br>--<br>Michihiro,<br>IBM Research - Tokyo</font><p><p><BR>
</body></html>