From 18a6a4ff8bef03459d1014e201783c85e8a23843 Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Fri, 25 Jul 2025 13:48:32 +0200 Subject: [PATCH 1/5] Fix --- .../share/oops/instanceMirrorKlass.hpp | 3 + .../share/oops/instanceMirrorKlass.inline.hpp | 58 +++++++++---------- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/hotspot/share/oops/instanceMirrorKlass.hpp b/src/hotspot/share/oops/instanceMirrorKlass.hpp index 9783d416a1d1b..63bed9738f82b 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.hpp @@ -52,6 +52,9 @@ class InstanceMirrorKlass: public InstanceKlass { InstanceMirrorKlass(const ClassFileParser& parser) : InstanceKlass(parser, Kind) {} + template + void do_metadata(oop obj, OopClosureType* closure); + public: InstanceMirrorKlass(); diff --git a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp index 867a0580a126d..bae706857640a 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp @@ -46,38 +46,36 @@ void InstanceMirrorKlass::oop_oop_iterate_statics(oop obj, OopClosureType* closu } } +template +void InstanceMirrorKlass::do_metadata(oop obj, OopClosureType* closure) { + Klass* klass = java_lang_Class::as_Klass(obj); + if (klass != nullptr) { + if (klass->class_loader_data() == nullptr) { + // This is a mirror that belongs to a shared class that has not been loaded yet. + assert(klass->is_shared(), "must be"); + } else if (klass->is_instance_klass() && klass->class_loader_data()->has_class_mirror_holder()) { + // A non-strong hidden class doesn't have its own class loader, + // so when handling the java mirror for the class we need to make sure its class + // loader data is claimed, this is done by calling do_cld explicitly. + // For non-strong hidden classes the call to do_cld is made when the class + // loader itself is handled. + Devirtualizer::do_cld(closure, klass->class_loader_data()); + } else { + Devirtualizer::do_klass(closure, klass); + } + } else { + // Klass is null means this has been a mirror for a primitive type + // that we do not need to follow as they are always strong roots. + assert(java_lang_Class::is_primitive(obj), "Sanity check"); + } +} + template void InstanceMirrorKlass::oop_oop_iterate(oop obj, OopClosureType* closure) { InstanceKlass::oop_oop_iterate(obj, closure); if (Devirtualizer::do_metadata(closure)) { - Klass* klass = java_lang_Class::as_Klass(obj); - // We'll get null for primitive mirrors. - if (klass != nullptr) { - if (klass->class_loader_data() == nullptr) { - // This is a mirror that belongs to a shared class that has not be loaded yet. - assert(klass->is_shared(), "must be"); - } else if (klass->is_instance_klass() && klass->class_loader_data()->has_class_mirror_holder()) { - // A non-strong hidden class doesn't have its own class loader, - // so when handling the java mirror for the class we need to make sure its class - // loader data is claimed, this is done by calling do_cld explicitly. - // For non-strong hidden classes the call to do_cld is made when the class - // loader itself is handled. - Devirtualizer::do_cld(closure, klass->class_loader_data()); - } else { - Devirtualizer::do_klass(closure, klass); - } - } else { - // We would like to assert here (as below) that if klass has been null, then - // this has been a mirror for a primitive type that we do not need to follow - // as they are always strong roots. - // However, we might get across a klass that just changed during CMS concurrent - // marking if allocation occurred in the old generation. - // This is benign here, as we keep alive all CLDs that were loaded during the - // CMS concurrent phase in the class loading, i.e. they will be iterated over - // and kept alive during remark. - // assert(java_lang_Class::is_primitive(obj), "Sanity check"); - } + do_metadata(obj, closure); } oop_oop_iterate_statics(obj, closure); @@ -121,11 +119,7 @@ void InstanceMirrorKlass::oop_oop_iterate_bounded(oop obj, OopClosureType* closu if (Devirtualizer::do_metadata(closure)) { if (mr.contains(obj)) { - Klass* klass = java_lang_Class::as_Klass(obj); - // We'll get null for primitive mirrors. - if (klass != nullptr) { - Devirtualizer::do_klass(closure, klass); - } + do_metadata(obj, closure); } } From 8460be7d108712886caf82c3e45d5fffdca469aa Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Fri, 25 Jul 2025 13:57:05 +0200 Subject: [PATCH 2/5] Mark as inline as well --- src/hotspot/share/oops/instanceMirrorKlass.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hotspot/share/oops/instanceMirrorKlass.hpp b/src/hotspot/share/oops/instanceMirrorKlass.hpp index 63bed9738f82b..e9928647db9a4 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.hpp @@ -53,7 +53,7 @@ class InstanceMirrorKlass: public InstanceKlass { InstanceMirrorKlass(const ClassFileParser& parser) : InstanceKlass(parser, Kind) {} template - void do_metadata(oop obj, OopClosureType* closure); + inline void do_metadata(oop obj, OopClosureType* closure); public: InstanceMirrorKlass(); From 5e4153ae0d3556d41182f1987ec28c28668a3539 Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Fri, 25 Jul 2025 13:59:33 +0200 Subject: [PATCH 3/5] Comments --- src/hotspot/share/oops/instanceMirrorKlass.inline.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp index bae706857640a..9a37a9f400db5 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp @@ -52,7 +52,7 @@ void InstanceMirrorKlass::do_metadata(oop obj, OopClosureType* closure) { if (klass != nullptr) { if (klass->class_loader_data() == nullptr) { // This is a mirror that belongs to a shared class that has not been loaded yet. - assert(klass->is_shared(), "must be"); + assert(klass->is_shared(), "Must be"); } else if (klass->is_instance_klass() && klass->class_loader_data()->has_class_mirror_holder()) { // A non-strong hidden class doesn't have its own class loader, // so when handling the java mirror for the class we need to make sure its class @@ -66,7 +66,7 @@ void InstanceMirrorKlass::do_metadata(oop obj, OopClosureType* closure) { } else { // Klass is null means this has been a mirror for a primitive type // that we do not need to follow as they are always strong roots. - assert(java_lang_Class::is_primitive(obj), "Sanity check"); + assert(java_lang_Class::is_primitive(obj), "Sanity"); } } From bc85328794cebb8ccda7f75e4aeb75b7459b16ad Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Fri, 25 Jul 2025 19:20:07 +0200 Subject: [PATCH 4/5] Drop assert, more discussion --- src/hotspot/share/oops/instanceMirrorKlass.inline.hpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp index 9a37a9f400db5..945a963544ddc 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp @@ -64,9 +64,14 @@ void InstanceMirrorKlass::do_metadata(oop obj, OopClosureType* closure) { Devirtualizer::do_klass(closure, klass); } } else { - // Klass is null means this has been a mirror for a primitive type - // that we do not need to follow as they are always strong roots. - assert(java_lang_Class::is_primitive(obj), "Sanity"); + // Java mirror -> Klass* "nullptr" backlink means either: + // 1. obj is a Java mirror for a primitive class, we do not need to follow it, + // these mirrors are always strong roots. + // 2. obj is a Java mirror for a newly allocated non-primitive class, and we + // somehow managed to reach the newly allocated Java mirror with not yet + // installed backlink. This case should be made benign by GC itself for + // this and any other Java mirror usage path, we cannot do anything here. + // Unfortunately, the existence of corner case (2) precludes asserting (1). } } From 40761ecc255614d395c2fcc3a2ea5c2ea0c5eaee Mon Sep 17 00:00:00 2001 From: Aleksey Shipilev Date: Mon, 28 Jul 2025 12:46:27 +0200 Subject: [PATCH 5/5] Better comment --- src/hotspot/share/oops/instanceMirrorKlass.inline.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp index 945a963544ddc..eed87d2644b7e 100644 --- a/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp +++ b/src/hotspot/share/oops/instanceMirrorKlass.inline.hpp @@ -65,13 +65,13 @@ void InstanceMirrorKlass::do_metadata(oop obj, OopClosureType* closure) { } } else { // Java mirror -> Klass* "nullptr" backlink means either: - // 1. obj is a Java mirror for a primitive class, we do not need to follow it, + // 1. This is a Java mirror for a primitive class. We do not need to follow it, // these mirrors are always strong roots. - // 2. obj is a Java mirror for a newly allocated non-primitive class, and we + // 2. This is a Java mirror for a newly allocated non-primitive class, and we // somehow managed to reach the newly allocated Java mirror with not yet - // installed backlink. This case should be made benign by GC itself for - // this and any other Java mirror usage path, we cannot do anything here. - // Unfortunately, the existence of corner case (2) precludes asserting (1). + // installed backlink. We cannot do anything here, this case would be handled + // separately by GC, e.g. by keeping the relevant metadata alive during the GC. + // Unfortunately, the existence of corner case (2) prevents us from asserting (1). } }