From eae437f1f84e0253af2a7dcf1acc14e897cf8140 Mon Sep 17 00:00:00 2001 From: coleenp Date: Mon, 24 Jun 2013 18:55:46 -0400 Subject: [PATCH] 8016325: JVM hangs verifying system dictionary Summary: Minimize redundant verifications of Klasses. Reviewed-by: hseigel, jmasa --- src/cpu/sparc/vm/frame_sparc.cpp | 2 +- src/cpu/x86/vm/frame_x86.cpp | 2 +- src/share/vm/ci/ciObjectFactory.cpp | 2 -- src/share/vm/classfile/dictionary.cpp | 2 +- src/share/vm/code/debugInfo.hpp | 2 +- src/share/vm/code/dependencies.cpp | 4 +-- .../vm/gc_implementation/g1/heapRegion.cpp | 2 +- src/share/vm/memory/allocation.cpp | 7 ----- src/share/vm/memory/allocation.hpp | 1 - src/share/vm/memory/heapInspection.cpp | 1 - src/share/vm/oops/arrayKlass.cpp | 4 +-- src/share/vm/oops/arrayKlass.hpp | 2 +- src/share/vm/oops/compiledICHolder.cpp | 2 -- src/share/vm/oops/constMethod.cpp | 1 - src/share/vm/oops/constantPool.cpp | 4 --- src/share/vm/oops/instanceKlass.cpp | 27 +++++++------------ src/share/vm/oops/instanceKlass.hpp | 2 +- src/share/vm/oops/klass.cpp | 11 +++----- src/share/vm/oops/klass.hpp | 4 +-- src/share/vm/oops/method.cpp | 5 ---- src/share/vm/oops/objArrayKlass.cpp | 6 ++--- src/share/vm/oops/objArrayKlass.hpp | 2 +- src/share/vm/runtime/frame.cpp | 1 - src/share/vm/shark/sharkBuilder.cpp | 2 +- 24 files changed, 30 insertions(+), 68 deletions(-) diff --git a/src/cpu/sparc/vm/frame_sparc.cpp b/src/cpu/sparc/vm/frame_sparc.cpp index b550d77a8..55f344f55 100644 --- a/src/cpu/sparc/vm/frame_sparc.cpp +++ b/src/cpu/sparc/vm/frame_sparc.cpp @@ -680,7 +680,7 @@ bool frame::is_interpreted_frame_valid(JavaThread* thread) const { // validate ConstantPoolCache* ConstantPoolCache* cp = *interpreter_frame_cache_addr(); - if (cp == NULL || !cp->is_metadata()) return false; + if (cp == NULL || !cp->is_metaspace_object()) return false; // validate locals diff --git a/src/cpu/x86/vm/frame_x86.cpp b/src/cpu/x86/vm/frame_x86.cpp index 92587985d..c2a7c8b49 100644 --- a/src/cpu/x86/vm/frame_x86.cpp +++ b/src/cpu/x86/vm/frame_x86.cpp @@ -587,7 +587,7 @@ bool frame::is_interpreted_frame_valid(JavaThread* thread) const { // validate ConstantPoolCache* ConstantPoolCache* cp = *interpreter_frame_cache_addr(); - if (cp == NULL || !cp->is_metadata()) return false; + if (cp == NULL || !cp->is_metaspace_object()) return false; // validate locals diff --git a/src/share/vm/ci/ciObjectFactory.cpp b/src/share/vm/ci/ciObjectFactory.cpp index 8fb6e12b7..cc1be034d 100644 --- a/src/share/vm/ci/ciObjectFactory.cpp +++ b/src/share/vm/ci/ciObjectFactory.cpp @@ -265,8 +265,6 @@ ciObject* ciObjectFactory::get(oop key) { ciMetadata* ciObjectFactory::get_metadata(Metadata* key) { ASSERT_IN_VM; - assert(key == NULL || key->is_metadata(), "must be"); - #ifdef ASSERT if (CIObjectFactoryVerify) { Metadata* last = NULL; diff --git a/src/share/vm/classfile/dictionary.cpp b/src/share/vm/classfile/dictionary.cpp index b79cda74f..26e06c8a0 100644 --- a/src/share/vm/classfile/dictionary.cpp +++ b/src/share/vm/classfile/dictionary.cpp @@ -555,7 +555,7 @@ void Dictionary::verify() { loader_data->class_loader() == NULL || loader_data->class_loader()->is_instance(), "checking type of class_loader"); - e->verify(); + e->verify(/*check_dictionary*/false); probe->verify_protection_domain_set(); element_count++; } diff --git a/src/share/vm/code/debugInfo.hpp b/src/share/vm/code/debugInfo.hpp index 3f65652d8..7a4f7e2d4 100644 --- a/src/share/vm/code/debugInfo.hpp +++ b/src/share/vm/code/debugInfo.hpp @@ -274,7 +274,7 @@ class DebugInfoReadStream : public CompressedReadStream { Method* read_method() { Method* o = (Method*)(code()->metadata_at(read_int())); assert(o == NULL || - o->is_metadata(), "meta data only"); + o->is_metaspace_object(), "meta data only"); return o; } ScopeValue* read_object_value(); diff --git a/src/share/vm/code/dependencies.cpp b/src/share/vm/code/dependencies.cpp index 4db37819e..df269430e 100644 --- a/src/share/vm/code/dependencies.cpp +++ b/src/share/vm/code/dependencies.cpp @@ -655,8 +655,8 @@ inline Metadata* Dependencies::DepStream::recorded_metadata_at(int i) { } else { o = _deps->oop_recorder()->metadata_at(i); } - assert(o == NULL || o->is_metadata(), - err_msg("Should be perm " PTR_FORMAT, o)); + assert(o == NULL || o->is_metaspace_object(), + err_msg("Should be metadata " PTR_FORMAT, o)); return o; } diff --git a/src/share/vm/gc_implementation/g1/heapRegion.cpp b/src/share/vm/gc_implementation/g1/heapRegion.cpp index 7ef24358e..22c84ec81 100644 --- a/src/share/vm/gc_implementation/g1/heapRegion.cpp +++ b/src/share/vm/gc_implementation/g1/heapRegion.cpp @@ -798,7 +798,7 @@ void HeapRegion::verify(VerifyOption vo, if (!g1->is_obj_dead_cond(obj, this, vo)) { if (obj->is_oop()) { Klass* klass = obj->klass(); - if (!klass->is_metadata()) { + if (!klass->is_metaspace_object()) { gclog_or_tty->print_cr("klass "PTR_FORMAT" of object "PTR_FORMAT" " "not metadata", klass, obj); *failures = true; diff --git a/src/share/vm/memory/allocation.cpp b/src/share/vm/memory/allocation.cpp index 6b08c13e5..5d934fa29 100644 --- a/src/share/vm/memory/allocation.cpp +++ b/src/share/vm/memory/allocation.cpp @@ -71,13 +71,6 @@ bool MetaspaceObj::is_shared() const { return MetaspaceShared::is_in_shared_space(this); } -bool MetaspaceObj::is_metadata() const { - // GC Verify checks use this in guarantees. - // TODO: either replace them with is_metaspace_object() or remove them. - // is_metaspace_object() is slower than this test. This test doesn't - // seem very useful for metaspace objects anymore though. - return !Universe::heap()->is_in_reserved(this); -} bool MetaspaceObj::is_metaspace_object() const { return Metaspace::contains((void*)this); diff --git a/src/share/vm/memory/allocation.hpp b/src/share/vm/memory/allocation.hpp index 4f5b7d9f9..011c98068 100644 --- a/src/share/vm/memory/allocation.hpp +++ b/src/share/vm/memory/allocation.hpp @@ -264,7 +264,6 @@ class ClassLoaderData; class MetaspaceObj { public: - bool is_metadata() const; bool is_metaspace_object() const; // more specific test but slower bool is_shared() const; void print_address_on(outputStream* st) const; // nonvirtual address printing diff --git a/src/share/vm/memory/heapInspection.cpp b/src/share/vm/memory/heapInspection.cpp index 2afecb680..bf65c882c 100644 --- a/src/share/vm/memory/heapInspection.cpp +++ b/src/share/vm/memory/heapInspection.cpp @@ -157,7 +157,6 @@ KlassInfoTable::~KlassInfoTable() { } uint KlassInfoTable::hash(const Klass* p) { - assert(p->is_metadata(), "all klasses are metadata"); return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2); } diff --git a/src/share/vm/oops/arrayKlass.cpp b/src/share/vm/oops/arrayKlass.cpp index dc2fbe92e..ef1c20e97 100644 --- a/src/share/vm/oops/arrayKlass.cpp +++ b/src/share/vm/oops/arrayKlass.cpp @@ -221,8 +221,8 @@ void ArrayKlass::oop_print_on(oop obj, outputStream* st) { // Verification -void ArrayKlass::verify_on(outputStream* st) { - Klass::verify_on(st); +void ArrayKlass::verify_on(outputStream* st, bool check_dictionary) { + Klass::verify_on(st, check_dictionary); if (component_mirror() != NULL) { guarantee(component_mirror()->klass() != NULL, "should have a class"); diff --git a/src/share/vm/oops/arrayKlass.hpp b/src/share/vm/oops/arrayKlass.hpp index f37a4d500..acf920e16 100644 --- a/src/share/vm/oops/arrayKlass.hpp +++ b/src/share/vm/oops/arrayKlass.hpp @@ -152,7 +152,7 @@ class ArrayKlass: public Klass { void oop_print_on(oop obj, outputStream* st); // Verification - void verify_on(outputStream* st); + void verify_on(outputStream* st, bool check_dictionary); void oop_verify_on(oop obj, outputStream* st); }; diff --git a/src/share/vm/oops/compiledICHolder.cpp b/src/share/vm/oops/compiledICHolder.cpp index c13b7559a..2b2cd2ae6 100644 --- a/src/share/vm/oops/compiledICHolder.cpp +++ b/src/share/vm/oops/compiledICHolder.cpp @@ -48,8 +48,6 @@ void CompiledICHolder::print_value_on(outputStream* st) const { // Verification void CompiledICHolder::verify_on(outputStream* st) { - guarantee(holder_method()->is_metadata(), "should be in metaspace"); guarantee(holder_method()->is_method(), "should be method"); - guarantee(holder_klass()->is_metadata(), "should be in metaspace"); guarantee(holder_klass()->is_klass(), "should be klass"); } diff --git a/src/share/vm/oops/constMethod.cpp b/src/share/vm/oops/constMethod.cpp index 22f3b87da..4c0720908 100644 --- a/src/share/vm/oops/constMethod.cpp +++ b/src/share/vm/oops/constMethod.cpp @@ -440,7 +440,6 @@ void ConstMethod::collect_statistics(KlassSizeStats *sz) const { void ConstMethod::verify_on(outputStream* st) { guarantee(is_constMethod(), "object must be constMethod"); - guarantee(is_metadata(), err_msg("Should be metadata " PTR_FORMAT, this)); // Verification can occur during oop construction before the method or // other fields have been initialized. diff --git a/src/share/vm/oops/constantPool.cpp b/src/share/vm/oops/constantPool.cpp index c2879e2d0..8033d7e38 100644 --- a/src/share/vm/oops/constantPool.cpp +++ b/src/share/vm/oops/constantPool.cpp @@ -2095,12 +2095,10 @@ void ConstantPool::verify_on(outputStream* st) { CPSlot entry = slot_at(i); if (tag.is_klass()) { if (entry.is_resolved()) { - guarantee(entry.get_klass()->is_metadata(), "should be metadata"); guarantee(entry.get_klass()->is_klass(), "should be klass"); } } else if (tag.is_unresolved_klass()) { if (entry.is_resolved()) { - guarantee(entry.get_klass()->is_metadata(), "should be metadata"); guarantee(entry.get_klass()->is_klass(), "should be klass"); } } else if (tag.is_symbol()) { @@ -2112,13 +2110,11 @@ void ConstantPool::verify_on(outputStream* st) { if (cache() != NULL) { // Note: cache() can be NULL before a class is completely setup or // in temporary constant pools used during constant pool merging - guarantee(cache()->is_metadata(), "should be metadata"); guarantee(cache()->is_constantPoolCache(), "should be constant pool cache"); } if (pool_holder() != NULL) { // Note: pool_holder() can be NULL in temporary constant pools // used during constant pool merging - guarantee(pool_holder()->is_metadata(), "should be metadata"); guarantee(pool_holder()->is_klass(), "should be klass"); } } diff --git a/src/share/vm/oops/instanceKlass.cpp b/src/share/vm/oops/instanceKlass.cpp index 84f1cdd3e..886746d6b 100644 --- a/src/share/vm/oops/instanceKlass.cpp +++ b/src/share/vm/oops/instanceKlass.cpp @@ -3088,27 +3088,26 @@ class VerifyFieldClosure: public OopClosure { virtual void do_oop(narrowOop* p) { VerifyFieldClosure::do_oop_work(p); } }; -void InstanceKlass::verify_on(outputStream* st) { - Klass::verify_on(st); - Thread *thread = Thread::current(); - +void InstanceKlass::verify_on(outputStream* st, bool check_dictionary) { #ifndef PRODUCT - // Avoid redundant verifies + // Avoid redundant verifies, this really should be in product. if (_verify_count == Universe::verify_count()) return; _verify_count = Universe::verify_count(); #endif - // Verify that klass is present in SystemDictionary - if (is_loaded() && !is_anonymous()) { + + // Verify Klass + Klass::verify_on(st, check_dictionary); + + // Verify that klass is present in SystemDictionary if not already + // verifying the SystemDictionary. + if (is_loaded() && !is_anonymous() && check_dictionary) { Symbol* h_name = name(); SystemDictionary::verify_obj_klass_present(h_name, class_loader_data()); } - // Verify static fields - VerifyFieldClosure blk; - // Verify vtables if (is_linked()) { - ResourceMark rm(thread); + ResourceMark rm; // $$$ This used to be done only for m/s collections. Doing it // always seemed a valid generalization. (DLD -- 6/00) vtable()->verify(st); @@ -3116,7 +3115,6 @@ void InstanceKlass::verify_on(outputStream* st) { // Verify first subklass if (subklass_oop() != NULL) { - guarantee(subklass_oop()->is_metadata(), "should be in metaspace"); guarantee(subklass_oop()->is_klass(), "should be klass"); } @@ -3128,7 +3126,6 @@ void InstanceKlass::verify_on(outputStream* st) { fatal(err_msg("subclass points to itself " PTR_FORMAT, sib)); } - guarantee(sib->is_metadata(), "should be in metaspace"); guarantee(sib->is_klass(), "should be klass"); guarantee(sib->super() == super, "siblings should have same superklass"); } @@ -3164,7 +3161,6 @@ void InstanceKlass::verify_on(outputStream* st) { if (methods() != NULL) { Array* methods = this->methods(); for (int j = 0; j < methods->length(); j++) { - guarantee(methods->at(j)->is_metadata(), "should be in metaspace"); guarantee(methods->at(j)->is_method(), "non-method in methods array"); } for (int j = 0; j < methods->length() - 1; j++) { @@ -3202,16 +3198,13 @@ void InstanceKlass::verify_on(outputStream* st) { // Verify other fields if (array_klasses() != NULL) { - guarantee(array_klasses()->is_metadata(), "should be in metaspace"); guarantee(array_klasses()->is_klass(), "should be klass"); } if (constants() != NULL) { - guarantee(constants()->is_metadata(), "should be in metaspace"); guarantee(constants()->is_constantPool(), "should be constant pool"); } const Klass* host = host_klass(); if (host != NULL) { - guarantee(host->is_metadata(), "should be in metaspace"); guarantee(host->is_klass(), "should be klass"); } } diff --git a/src/share/vm/oops/instanceKlass.hpp b/src/share/vm/oops/instanceKlass.hpp index 7d09f8132..6c56a5195 100644 --- a/src/share/vm/oops/instanceKlass.hpp +++ b/src/share/vm/oops/instanceKlass.hpp @@ -1050,7 +1050,7 @@ public: const char* internal_name() const; // Verification - void verify_on(outputStream* st); + void verify_on(outputStream* st, bool check_dictionary); void oop_verify_on(oop obj, outputStream* st); }; diff --git a/src/share/vm/oops/klass.cpp b/src/share/vm/oops/klass.cpp index 473942556..a719b88bb 100644 --- a/src/share/vm/oops/klass.cpp +++ b/src/share/vm/oops/klass.cpp @@ -377,7 +377,6 @@ void Klass::append_to_sibling_list() { } bool Klass::is_loader_alive(BoolObjectClosure* is_alive) { - assert(is_metadata(), "p is not meta-data"); assert(ClassLoaderDataGraph::contains((address)this), "is in the metaspace"); #ifdef ASSERT @@ -648,27 +647,24 @@ void Klass::collect_statistics(KlassSizeStats *sz) const { // Verification -void Klass::verify_on(outputStream* st) { - guarantee(!Universe::heap()->is_in_reserved(this), "Shouldn't be"); - guarantee(this->is_metadata(), "should be in metaspace"); +void Klass::verify_on(outputStream* st, bool check_dictionary) { + // This can be expensive, but it is worth checking that this klass is actually + // in the CLD graph but not in production. assert(ClassLoaderDataGraph::contains((address)this), "Should be"); guarantee(this->is_klass(),"should be klass"); if (super() != NULL) { - guarantee(super()->is_metadata(), "should be in metaspace"); guarantee(super()->is_klass(), "should be klass"); } if (secondary_super_cache() != NULL) { Klass* ko = secondary_super_cache(); - guarantee(ko->is_metadata(), "should be in metaspace"); guarantee(ko->is_klass(), "should be klass"); } for ( uint i = 0; i < primary_super_limit(); i++ ) { Klass* ko = _primary_supers[i]; if (ko != NULL) { - guarantee(ko->is_metadata(), "should be in metaspace"); guarantee(ko->is_klass(), "should be klass"); } } @@ -680,7 +676,6 @@ void Klass::verify_on(outputStream* st) { void Klass::oop_verify_on(oop obj, outputStream* st) { guarantee(obj->is_oop(), "should be oop"); - guarantee(obj->klass()->is_metadata(), "should not be in Java heap"); guarantee(obj->klass()->is_klass(), "klass field is not a klass"); } diff --git a/src/share/vm/oops/klass.hpp b/src/share/vm/oops/klass.hpp index 6b0103abb..5f4094d4f 100644 --- a/src/share/vm/oops/klass.hpp +++ b/src/share/vm/oops/klass.hpp @@ -703,8 +703,8 @@ class Klass : public Metadata { virtual const char* internal_name() const = 0; // Verification - virtual void verify_on(outputStream* st); - void verify() { verify_on(tty); } + virtual void verify_on(outputStream* st, bool check_dictionary); + void verify(bool check_dictionary = true) { verify_on(tty, check_dictionary); } #ifndef PRODUCT void verify_vtable_index(int index); diff --git a/src/share/vm/oops/method.cpp b/src/share/vm/oops/method.cpp index 1e8b1c3bb..5941efb3b 100644 --- a/src/share/vm/oops/method.cpp +++ b/src/share/vm/oops/method.cpp @@ -1969,14 +1969,9 @@ void Method::collect_statistics(KlassSizeStats *sz) const { void Method::verify_on(outputStream* st) { guarantee(is_method(), "object must be method"); - guarantee(is_metadata(), "should be metadata"); guarantee(constants()->is_constantPool(), "should be constant pool"); - guarantee(constants()->is_metadata(), "should be metadata"); guarantee(constMethod()->is_constMethod(), "should be ConstMethod*"); - guarantee(constMethod()->is_metadata(), "should be metadata"); MethodData* md = method_data(); - guarantee(md == NULL || - md->is_metadata(), "should be metadata"); guarantee(md == NULL || md->is_methodData(), "should be method data"); } diff --git a/src/share/vm/oops/objArrayKlass.cpp b/src/share/vm/oops/objArrayKlass.cpp index f04020639..f2f349106 100644 --- a/src/share/vm/oops/objArrayKlass.cpp +++ b/src/share/vm/oops/objArrayKlass.cpp @@ -676,11 +676,9 @@ const char* ObjArrayKlass::internal_name() const { // Verification -void ObjArrayKlass::verify_on(outputStream* st) { - ArrayKlass::verify_on(st); - guarantee(element_klass()->is_metadata(), "should be in metaspace"); +void ObjArrayKlass::verify_on(outputStream* st, bool check_dictionary) { + ArrayKlass::verify_on(st, check_dictionary); guarantee(element_klass()->is_klass(), "should be klass"); - guarantee(bottom_klass()->is_metadata(), "should be in metaspace"); guarantee(bottom_klass()->is_klass(), "should be klass"); Klass* bk = bottom_klass(); guarantee(bk->oop_is_instance() || bk->oop_is_typeArray(), "invalid bottom klass"); diff --git a/src/share/vm/oops/objArrayKlass.hpp b/src/share/vm/oops/objArrayKlass.hpp index 9905704cc..d56a3de84 100644 --- a/src/share/vm/oops/objArrayKlass.hpp +++ b/src/share/vm/oops/objArrayKlass.hpp @@ -151,7 +151,7 @@ class ObjArrayKlass : public ArrayKlass { const char* internal_name() const; // Verification - void verify_on(outputStream* st); + void verify_on(outputStream* st, bool check_dictionary); void oop_verify_on(oop obj, outputStream* st); }; diff --git a/src/share/vm/runtime/frame.cpp b/src/share/vm/runtime/frame.cpp index bf9d422b0..92af92e92 100644 --- a/src/share/vm/runtime/frame.cpp +++ b/src/share/vm/runtime/frame.cpp @@ -387,7 +387,6 @@ void frame::interpreter_frame_set_locals(intptr_t* locs) { Method* frame::interpreter_frame_method() const { assert(is_interpreted_frame(), "interpreted frame expected"); Method* m = *interpreter_frame_method_addr(); - assert(m->is_metadata(), "bad Method* in interpreter frame"); assert(m->is_method(), "not a Method*"); return m; } diff --git a/src/share/vm/shark/sharkBuilder.cpp b/src/share/vm/shark/sharkBuilder.cpp index f9c22bd16..8e83dd18e 100644 --- a/src/share/vm/shark/sharkBuilder.cpp +++ b/src/share/vm/shark/sharkBuilder.cpp @@ -471,7 +471,7 @@ Value* SharkBuilder::CreateInlineOop(jobject object, const char* name) { Value* SharkBuilder::CreateInlineMetadata(Metadata* metadata, llvm::PointerType* type, const char* name) { assert(metadata != NULL, "inlined metadata must not be NULL"); - assert(metadata->is_metadata(), "sanity check"); + assert(metadata->is_metaspace_object(), "sanity check"); return CreateLoad( CreateIntToPtr( code_buffer_address(code_buffer()->inline_Metadata(metadata)), -- GitLab