Hi Thomas, this looks like a nice cleanup. Thanks for improving. Your changes look correct. Best regards, Martin -----Original Message----- From: hotspot-gc-dev <hotspot-gc-dev-bounces@openjdk.java.net> On Behalf Of Thomas Schatzl Sent: Montag, 22. Oktober 2018 15:02 To: hotspot-gc-dev@openjdk.java.net Subject: RFR (XS): 8212753: Improve oopDesc::forward_to_atomic Hi all, can I have reviews for this micro-optimization that de-clutters oopDesc::forward_to_atomic used in CMS and G1? Reasons for this change: - the mark is reloaded in the method although all collectors could pass in their current copy (the mark is a volatile member, so it can't be optimized away) - it checks whether the mark is already set (i.e. already forwarded) before doing the CAS, and after doing the CAS. The former is waste of time because forwarding undo-statistics show that the number of failures (i.e. an object has already been forwarded) is extremely rate. - the check after the CAS whether the mark is set is superfluous, because after the CAS, independent of whether it was successful or not, we know that the mark must have been forwarded. There is no particular performance change. Just removes needlessly executed code. (Just before somebody asks: the two asserts in the changed forward_to_atomic() method are conditionalized on CMS because it will CAS in a special value into the mark word that do not fit the rest of the condition to "reserve" it for old gen promotion). CR: https://bugs.openjdk.java.net/browse/JDK-8212753 Webrev: http://cr.openjdk.java.net/~tschatzl/8212753/webrev/ Testing: hs-tier1-3 Thanks, Thomas

On Oct 22, 2018, at 9:02 AM, Thomas Schatzl <thomas.schatzl@oracle.com> wrote:

CR: https://bugs.openjdk.java.net/browse/JDK-8212753 Webrev: http://cr.openjdk.java.net/~tschatzl/8212753/webrev/ Testing: hs-tier1-3

Thanks, Thomas

There’s an additional change to the G1 code here: src/hotspot/share/gc/g1/g1ParScanThreadState.cpp 313 } else if (!obj->is_typeArray()) { which seems unrelated to the described change and unmentioned in the RFR. Is this change intentional here? Is this a measurable optimization? For the CMS-conditionalized asserts in forward_to_atomic, please add a comment that CMS may CAS in a special pseudo-oop value.