From 72b3f81dd4851a96bf91873fa212284c2a2d676d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Gr=C3=B6nlund?= Date: Tue, 29 Oct 2019 11:33:25 +0100 Subject: [PATCH] 8230400: Missing constant pool entry for a method in stacktrace Reviewed-by: egahlin --- .../jfrEventClassTransformer.cpp | 48 +------- .../checkpoint/jfrCheckpointManager.cpp | 12 +- .../recorder/checkpoint/types/jfrTypeSet.cpp | 110 ++++++++++++++---- .../checkpoint/types/jfrTypeSetUtils.hpp | 6 - .../types/traceid/jfrTraceIdBits.inline.hpp | 2 +- .../jdk/jfr/internal/MetadataRepository.java | 16 +-- 6 files changed, 106 insertions(+), 88 deletions(-) diff --git a/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp b/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp index 1cc451f0c32..fa40e07809a 100644 --- a/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp +++ b/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp @@ -1497,35 +1497,6 @@ static void rewrite_klass_pointer(InstanceKlass*& ik, InstanceKlass* new_ik, Cla ik = new_ik; } -// During retransform/redefine, copy the Method specific trace flags -// from the previous ik ("the original klass") to the new ik ("the scratch_klass"). -// The open code for retransform/redefine does not know about these. -// In doing this migration here, we ensure the new Methods (defined in scratch klass) -// will carry over trace tags from the old Methods being replaced, -// ensuring flag/tag continuity while being transparent to open code. -static void copy_method_trace_flags(const InstanceKlass* the_original_klass, const InstanceKlass* the_scratch_klass) { - assert(the_original_klass != NULL, "invariant"); - assert(the_scratch_klass != NULL, "invariant"); - assert(the_original_klass->name() == the_scratch_klass->name(), "invariant"); - const Array* old_methods = the_original_klass->methods(); - const Array* new_methods = the_scratch_klass->methods(); - const bool equal_array_length = old_methods->length() == new_methods->length(); - // The Method array has the property of being sorted. - // If they are the same length, there is a one-to-one mapping. - // If they are unequal, there was a method added (currently only - // private static methods allowed to be added), use lookup. - for (int i = 0; i < old_methods->length(); ++i) { - const Method* const old_method = old_methods->at(i); - Method* const new_method = equal_array_length ? new_methods->at(i) : - the_scratch_klass->find_method(old_method->name(), old_method->signature()); - assert(new_method != NULL, "invariant"); - assert(new_method->name() == old_method->name(), "invariant"); - assert(new_method->signature() == old_method->signature(), "invariant"); - new_method->set_trace_flags(old_method->trace_flags()); - assert(new_method->trace_flags() == old_method->trace_flags(), "invariant"); - } -} - static bool is_retransforming(const InstanceKlass* ik, TRAPS) { assert(ik != NULL, "invariant"); assert(JdkJfrEvent::is_a(ik), "invariant"); @@ -1533,16 +1504,7 @@ static bool is_retransforming(const InstanceKlass* ik, TRAPS) { assert(name != NULL, "invariant"); Handle class_loader(THREAD, ik->class_loader()); Handle protection_domain(THREAD, ik->protection_domain()); - // nota bene: use lock-free dictionary lookup - const InstanceKlass* prev_ik = (const InstanceKlass*)SystemDictionary::find(name, class_loader, protection_domain, THREAD); - if (prev_ik == NULL) { - return false; - } - // an existing ik implies a retransform/redefine - assert(prev_ik != NULL, "invariant"); - assert(JdkJfrEvent::is_a(prev_ik), "invariant"); - copy_method_trace_flags(prev_ik, ik); - return true; + return SystemDictionary::find(name, class_loader, protection_domain, THREAD) != NULL; } // target for JFR_ON_KLASS_CREATION hook @@ -1571,12 +1533,8 @@ void JfrEventClassTransformer::on_klass_creation(InstanceKlass*& ik, ClassFilePa return; } assert(JdkJfrEvent::is_subklass(ik), "invariant"); - if (is_retransforming(ik, THREAD)) { - // not the initial klass load - return; - } - if (ik->is_abstract()) { - // abstract classes are not instrumented + if (ik->is_abstract() || is_retransforming(ik, THREAD)) { + // abstract and scratch classes are not instrumented return; } ResourceMark rm(THREAD); diff --git a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp index fbbd59fb137..c40ab5f1d57 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp @@ -364,13 +364,13 @@ void JfrCheckpointManager::write_type_set() { if (!LeakProfiler::is_running()) { JfrCheckpointWriter writer(true, true, Thread::current()); JfrTypeSet::serialize(&writer, NULL, false); - return; + } else { + Thread* const t = Thread::current(); + JfrCheckpointWriter leakp_writer(false, true, t); + JfrCheckpointWriter writer(false, true, t); + JfrTypeSet::serialize(&writer, &leakp_writer, false); + ObjectSampleCheckpoint::on_type_set(leakp_writer); } - Thread* const t = Thread::current(); - JfrCheckpointWriter leakp_writer(false, true, t); - JfrCheckpointWriter writer(false, true, t); - JfrTypeSet::serialize(&writer, &leakp_writer, false); - ObjectSampleCheckpoint::on_type_set(leakp_writer); } void JfrCheckpointManager::write_type_set_for_unloaded_classes() { diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp index 3e777c5a252..168e47ec49b 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp @@ -43,6 +43,7 @@ #include "oops/objArrayKlass.hpp" #include "oops/oop.inline.hpp" #include "utilities/accessFlags.hpp" +#include "utilities/bitMap.inline.hpp" typedef const Klass* KlassPtr; typedef const PackageEntry* PkgPtr; @@ -117,7 +118,7 @@ static traceid package_id(KlassPtr klass, bool leakp) { static traceid module_id(PkgPtr pkg, bool leakp) { assert(pkg != NULL, "invariant"); ModPtr module_entry = pkg->module(); - if (module_entry == NULL || !module_entry->is_named()) { + if (module_entry == NULL) { return 0; } if (leakp) { @@ -136,9 +137,7 @@ static traceid method_id(KlassPtr klass, MethodPtr method) { static traceid cld_id(CldPtr cld, bool leakp) { assert(cld != NULL, "invariant"); - if (cld->is_unsafe_anonymous()) { - return 0; - } + assert(!cld->is_unsafe_anonymous(), "invariant"); if (leakp) { SET_LEAKP(cld); } else { @@ -153,6 +152,17 @@ static s4 get_flags(const T* ptr) { return ptr->access_flags().get_flags(); } +static bool is_unsafe_anonymous(const Klass* klass) { + assert(klass != NULL, "invariant"); + return klass->is_instance_klass() && ((const InstanceKlass*)klass)->is_unsafe_anonymous(); +} + +static ClassLoaderData* get_cld(const Klass* klass) { + assert(klass != NULL, "invariant"); + return is_unsafe_anonymous(klass) ? + InstanceKlass::cast(klass)->unsafe_anonymous_host()->class_loader_data() : klass->class_loader_data(); +} + template static void set_serialized(const T* ptr) { assert(ptr != NULL, "invariant"); @@ -184,7 +194,7 @@ static int write_klass(JfrCheckpointWriter* writer, KlassPtr klass, bool leakp) assert(theklass->is_typeArray_klass(), "invariant"); } writer->write(artifact_id(klass)); - writer->write(cld_id(klass->class_loader_data(), leakp)); + writer->write(cld_id(get_cld(klass), leakp)); writer->write(mark_symbol(klass, leakp)); writer->write(pkg_id); writer->write(get_flags(klass)); @@ -538,13 +548,22 @@ static void do_class_loader_data(ClassLoaderData* cld) { do_previous_epoch_artifact(_subsystem_callback, cld); } -class CldFieldSelector { +class KlassCldFieldSelector { public: typedef CldPtr TypePtr; static TypePtr select(KlassPtr klass) { assert(klass != NULL, "invariant"); - CldPtr cld = klass->class_loader_data(); - return cld->is_unsafe_anonymous() ? NULL : cld; + return get_cld(klass); + } +}; + +class ModuleCldFieldSelector { +public: + typedef CldPtr TypePtr; + static TypePtr select(KlassPtr klass) { + assert(klass != NULL, "invariant"); + ModPtr mod = ModuleFieldSelector::select(klass); + return mod != NULL ? mod->loader_data() : NULL; } }; @@ -570,14 +589,18 @@ typedef JfrPredicatedTypeWriterImplHost CldWriter; typedef CompositeFunctor > CldWriterWithClear; typedef JfrArtifactCallbackHost CldCallback; -typedef KlassToFieldEnvelope KlassCldWriter; +typedef KlassToFieldEnvelope KlassCldWriter; +typedef KlassToFieldEnvelope ModuleCldWriter; +typedef CompositeFunctor KlassAndModuleCldWriter; typedef LeakPredicate LeakCldPredicate; typedef JfrPredicatedTypeWriterImplHost LeakCldWriterImpl; typedef JfrTypeWriterHost LeakCldWriter; typedef CompositeFunctor CompositeCldWriter; -typedef KlassToFieldEnvelope KlassCompositeCldWriter; +typedef KlassToFieldEnvelope KlassCompositeCldWriter; +typedef KlassToFieldEnvelope ModuleCompositeCldWriter; +typedef CompositeFunctor KlassAndModuleCompositeCldWriter; typedef CompositeFunctor > CompositeCldWriterWithClear; typedef JfrArtifactCallbackHost CompositeCldCallback; @@ -585,14 +608,16 @@ static void write_classloaders() { assert(_writer != NULL, "invariant"); CldWriter cldw(_writer, _class_unload); KlassCldWriter kcw(&cldw); + ModuleCldWriter mcw(&cldw); + KlassAndModuleCldWriter kmcw(&kcw, &mcw); if (current_epoch()) { - _artifacts->iterate_klasses(kcw); + _artifacts->iterate_klasses(kmcw); _artifacts->tally(cldw); return; } assert(previous_epoch(), "invariant"); if (_leakp_writer == NULL) { - _artifacts->iterate_klasses(kcw); + _artifacts->iterate_klasses(kmcw); ClearArtifact clear; CldWriterWithClear cldwwc(&cldw, &clear); CldCallback callback(&cldwwc); @@ -602,7 +627,9 @@ static void write_classloaders() { LeakCldWriter lcldw(_leakp_writer, _class_unload); CompositeCldWriter ccldw(&lcldw, &cldw); KlassCompositeCldWriter kccldw(&ccldw); - _artifacts->iterate_klasses(kccldw); + ModuleCompositeCldWriter mccldw(&ccldw); + KlassAndModuleCompositeCldWriter kmccldw(&kccldw, &mccldw); + _artifacts->iterate_klasses(kmccldw); ClearArtifact clear; CompositeCldWriterWithClear ccldwwc(&ccldw, &clear); CompositeCldCallback callback(&ccldwwc); @@ -652,7 +679,31 @@ int write__method__leakp(JfrCheckpointWriter* writer, const void* m) { return write_method(writer, method, true); } -template +class BitMapFilter { + ResourceBitMap _bitmap; + public: + explicit BitMapFilter(int length = 0) : _bitmap((size_t)length) {} + bool operator()(size_t idx) { + if (_bitmap.size() == 0) { + return true; + } + if (_bitmap.at(idx)) { + return false; + } + _bitmap.set_bit(idx); + return true; + } +}; + +class AlwaysTrue { + public: + explicit AlwaysTrue(int length = 0) {} + bool operator()(size_t idx) { + return true; + } +}; + +template class MethodIteratorHost { private: MethodCallback _method_cb; @@ -671,13 +722,19 @@ class MethodIteratorHost { bool operator()(KlassPtr klass) { if (_method_used_predicate(klass)) { - const InstanceKlass* const ik = InstanceKlass::cast(klass); + const InstanceKlass* ik = InstanceKlass::cast(klass); const int len = ik->methods()->length(); - for (int i = 0; i < len; ++i) { - MethodPtr method = ik->methods()->at(i); - if (_method_flag_predicate(method)) { - _method_cb(method); + Filter filter(ik->previous_versions() != NULL ? len : 0); + while (ik != NULL) { + for (int i = 0; i < len; ++i) { + MethodPtr method = ik->methods()->at(i); + if (_method_flag_predicate(method) && filter(i)) { + _method_cb(method); + } } + // There can be multiple versions of the same method running + // due to redefinition. Need to inspect the complete set of methods. + ik = ik->previous_versions(); } } return _klass_cb(klass); @@ -697,16 +754,23 @@ class Wrapper { } }; +template +class EmptyStub { + public: + bool operator()(T const& value) { return true; } +}; + typedef SerializePredicate MethodPredicate; typedef JfrPredicatedTypeWriterImplHost MethodWriterImplTarget; -typedef Wrapper KlassCallbackStub; +typedef Wrapper KlassCallbackStub; typedef JfrTypeWriterHost MethodWriterImpl; -typedef MethodIteratorHost MethodWriter; +typedef MethodIteratorHost MethodWriter; typedef LeakPredicate LeakMethodPredicate; typedef JfrPredicatedTypeWriterImplHost LeakMethodWriterImplTarget; typedef JfrTypeWriterHost LeakMethodWriterImpl; -typedef MethodIteratorHost LeakMethodWriter; +typedef MethodIteratorHost LeakMethodWriter; +typedef MethodIteratorHost LeakMethodWriter; typedef CompositeFunctor CompositeMethodWriter; static void write_methods() { @@ -832,7 +896,7 @@ void JfrTypeSet::clear() { typedef Wrapper ClearKlassBits; typedef Wrapper ClearMethodFlag; -typedef MethodIteratorHost ClearKlassAndMethods; +typedef MethodIteratorHost ClearKlassAndMethods; static size_t teardown() { assert(_artifacts != NULL, "invariant"); diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSetUtils.hpp b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSetUtils.hpp index 0482489601f..f75c0ef0c7e 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSetUtils.hpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSetUtils.hpp @@ -98,12 +98,6 @@ class ClearArtifact { } }; -template -class Stub { - public: - bool operator()(T const& value) { return true; } -}; - template class SerializePredicate { bool _class_unload; diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp index ad4a3c1f5c5..9c2a5f2bf07 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp @@ -38,7 +38,7 @@ static const int low_offset = 7; static const int meta_offset = low_offset - 1; #endif -inline void set_bits(jbyte bits, jbyte* const dest) { +inline void set_bits(jbyte bits, jbyte volatile* const dest) { assert(dest != NULL, "invariant"); if (bits != (*dest & bits)) { *dest |= bits; diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataRepository.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataRepository.java index d27b2cd655c..a277086bcf6 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataRepository.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/MetadataRepository.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -255,20 +255,22 @@ public final class MetadataRepository { staleMetadata = true; } - // Lock around setOutput ensures that other threads dosn't - // emit event after setOutput and unregister the event class, before a call + // Lock around setOutput ensures that other threads don't + // emit events after setOutput and unregister the event class, before a call // to storeDescriptorInJVM synchronized void setOutput(String filename) { + if (staleMetadata) { + storeDescriptorInJVM(); + } jvm.setOutput(filename); unregisterUnloaded(); if (unregistered) { - staleMetadata = typeLibrary.clearUnregistered(); + if (typeLibrary.clearUnregistered()) { + storeDescriptorInJVM(); + } unregistered = false; } - if (staleMetadata) { - storeDescriptorInJVM(); - } } private void unregisterUnloaded() {