Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258414: OldObjectSample events too expensive #2645

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/hotspot/share/jfr/jni/jfrJniMethod.cpp
Expand Up @@ -251,7 +251,7 @@ JVM_ENTRY_NO_ENV(jlong, jfr_class_id(JNIEnv* env, jclass jvm, jclass jc))
JVM_END

JVM_ENTRY_NO_ENV(jlong, jfr_stacktrace_id(JNIEnv* env, jobject jvm, jint skip))
return JfrStackTraceRepository::record(thread, skip);
return JfrStackTraceRepository::instance().record(thread, skip);
JVM_END

JVM_ENTRY_NO_ENV(void, jfr_log(JNIEnv* env, jobject jvm, jint tag_set, jint level, jstring message))
Expand Down
Expand Up @@ -197,6 +197,57 @@ static bool stack_trace_precondition(const ObjectSample* sample) {
return sample->has_stack_trace_id() && !sample->is_dead();
}

class StackTraceChunkWriter {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, a detailed comment explaining how this is working with the regular stacktrace writer would be good to have here.

private:
const JfrStackTraceRepository& _stack_trace_repo;
JfrChunkWriter& _chunkwriter;
int _count;
const JfrStackTrace* resolve(const ObjectSample* sample);
public:
StackTraceChunkWriter(const JfrStackTraceRepository& stack_trace_repo, JfrChunkWriter& chunkwriter);
void sample_do(ObjectSample* sample) {
if (stack_trace_precondition(sample)) {
const JfrStackTrace* const stack_trace = resolve(sample);
if (stack_trace->should_write()) {
stack_trace->write(_chunkwriter);
_count++;
}
}
}
int count() const {
return _count;
}
};

StackTraceChunkWriter::StackTraceChunkWriter(const JfrStackTraceRepository& stack_trace_repo, JfrChunkWriter& chunkwriter) :
_stack_trace_repo(stack_trace_repo), _chunkwriter(chunkwriter), _count(0) {
}

const JfrStackTrace* StackTraceChunkWriter::resolve(const ObjectSample* sample) {
return _stack_trace_repo.lookup(sample->stack_trace_hash(), sample->stack_trace_id());
}

static int do_write_stacktraces(const ObjectSampler* sampler, JfrStackTraceRepository& stack_trace_repo, JfrChunkWriter& chunkwriter) {
assert(sampler != NULL, "invariant");
const ObjectSample* const last = sampler->last();
if (last != sampler->last_resolved()) {
ResourceMark rm;
StackTraceChunkWriter writer(stack_trace_repo, chunkwriter);
iterate_samples(writer);
return writer.count();
}
return 0;
}

int ObjectSampleCheckpoint::write_objectsampler_stacktraces(const ObjectSampler* sampler, JfrStackTraceRepository& stack_trace_repo, JfrChunkWriter& chunkwriter) {
assert(sampler != NULL, "invariant");
assert(LeakProfiler::is_running(), "invariant");
MutexLocker lock(JfrStacktrace_lock, Mutex::_no_safepoint_check_flag);
// the lock is needed to ensure the unload lists do not grow in the middle of inspection.
return do_write_stacktraces(sampler, stack_trace_repo, chunkwriter);
}


class StackTraceBlobInstaller {
private:
const JfrStackTraceRepository& _stack_trace_repo;
Expand Down
Expand Up @@ -32,6 +32,7 @@ class EdgeStore;
class InstanceKlass;
class JavaThread;
class JfrCheckpointWriter;
class JfrChunkWriter;
class JfrStackTrace;
class JfrStackTraceRepository;
class Klass;
Expand All @@ -54,6 +55,7 @@ class ObjectSampleCheckpoint : AllStatic {
static void on_type_set_unload(JfrCheckpointWriter& writer);
static void on_thread_exit(JavaThread* jt);
static void on_rotation(const ObjectSampler* sampler, JfrStackTraceRepository& repo);
static int write_objectsampler_stacktraces(const ObjectSampler* sampler, JfrStackTraceRepository& stack_trace_repo, JfrChunkWriter& chunkwriter);
};

#endif // SHARE_JFR_LEAKPROFILER_CHECKPOINT_OBJECTSAMPLECHECKPOINT_HPP
Expand Up @@ -162,7 +162,7 @@ static traceid get_thread_id(JavaThread* thread) {
static void record_stacktrace(JavaThread* thread) {
assert(thread != NULL, "invariant");
if (JfrEventSetting::has_stacktrace(EventOldObjectSample::eventId)) {
JfrStackTraceRepository::record_and_cache(thread);
JfrStackTraceRepository::leak_profiler_instance().record_and_cache(thread);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
Expand Up @@ -261,7 +261,7 @@ bool JfrThreadSampleClosure::sample_thread_in_java(JavaThread* thread, JfrStackF
return false;
}
EventExecutionSample *event = &_events[_added_java - 1];
traceid id = JfrStackTraceRepository::add(sampler.stacktrace());
traceid id = JfrStackTraceRepository::instance().add(sampler.stacktrace());
assert(id != 0, "Stacktrace id should not be 0");
event->set_stackTrace(id);
return true;
Expand All @@ -281,7 +281,7 @@ bool JfrThreadSampleClosure::sample_thread_in_native(JavaThread* thread, JfrStac
return false;
}
EventNativeMethodSample *event = &_events_native[_added_native - 1];
traceid id = JfrStackTraceRepository::add(cb.stacktrace());
traceid id = JfrStackTraceRepository::instance().add(cb.stacktrace());
assert(id != 0, "Stacktrace id should not be 0");
event->set_stackTrace(id);
return true;
Expand Down
14 changes: 14 additions & 0 deletions src/hotspot/share/jfr/recorder/jfrRecorder.cpp
Expand Up @@ -281,6 +281,9 @@ bool JfrRecorder::create_components() {
if (!create_stacktrace_repository()) {
return false;
}
if (!create_leak_profiler_stacktrace_repository()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here explaining the purpose of having a separate leak profiler stacktrace repository?

return false;
}
if (!create_os_interface()) {
return false;
}
Expand All @@ -302,6 +305,7 @@ static JfrStorage* _storage = NULL;
static JfrCheckpointManager* _checkpoint_manager = NULL;
static JfrRepository* _repository = NULL;
static JfrStackTraceRepository* _stack_trace_repository;
static JfrStackTraceRepository* _stack_trace_repository_leak_profiler;
static JfrStringPool* _stringpool = NULL;
static JfrOSInterface* _os_interface = NULL;
static JfrThreadSampling* _thread_sampling = NULL;
Expand Down Expand Up @@ -353,6 +357,12 @@ bool JfrRecorder::create_stacktrace_repository() {
return _stack_trace_repository != NULL && _stack_trace_repository->initialize();
}

bool JfrRecorder::create_leak_profiler_stacktrace_repository() {
assert(_stack_trace_repository_leak_profiler == NULL, "invariant");
_stack_trace_repository_leak_profiler = JfrStackTraceRepository::create_leak_profiler();
return _stack_trace_repository_leak_profiler != NULL && _stack_trace_repository_leak_profiler->initialize();
}

bool JfrRecorder::create_stringpool() {
assert(_stringpool == NULL, "invariant");
assert(_repository != NULL, "invariant");
Expand Down Expand Up @@ -392,6 +402,10 @@ void JfrRecorder::destroy_components() {
JfrStackTraceRepository::destroy();
_stack_trace_repository = NULL;
}
if (_stack_trace_repository_leak_profiler != NULL) {
JfrStackTraceRepository::destroy_leak_profiler();
_stack_trace_repository_leak_profiler = NULL;
}
if (_stringpool != NULL) {
JfrStringPool::destroy();
_stringpool = NULL;
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/jfr/recorder/jfrRecorder.hpp
Expand Up @@ -50,6 +50,7 @@ class JfrRecorder : public JfrCHeapObj {
static bool create_post_box();
static bool create_recorder_thread();
static bool create_stacktrace_repository();
static bool create_leak_profiler_stacktrace_repository();
static bool create_storage();
static bool create_stringpool();
static bool create_thread_sampling();
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/jfr/recorder/service/jfrEvent.hpp
Expand Up @@ -227,7 +227,7 @@ class JfrEvent {
if (tl->has_cached_stack_trace()) {
writer.write(tl->cached_stack_trace_id());
} else {
writer.write(JfrStackTraceRepository::record(event_thread));
writer.write(JfrStackTraceRepository::instance().record(event_thread));
}
} else {
writer.write<traceid>(0);
Expand Down
60 changes: 59 additions & 1 deletion src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp
Expand Up @@ -264,6 +264,42 @@ static u4 invoke_with_flush_event(Functor& f) {
return elements;
}

class ObjectSamplerStackTraceRepository : public StackObj {
private:
JfrStackTraceRepository& _repo;
const ObjectSampler* _sampler;
JfrChunkWriter& _cw;
size_t _elements;
bool _clear;

public:
ObjectSamplerStackTraceRepository(JfrStackTraceRepository& repo, ObjectSampler* sampler, JfrChunkWriter& cw, bool clear) :
_repo(repo), _sampler(sampler), _cw(cw), _elements(0), _clear(clear) {}
bool process() {
_elements = ObjectSampleCheckpoint::write_objectsampler_stacktraces(_sampler, _repo, _cw);
if (_clear) {
_repo.clear();
}
return true;
}
size_t elements() const { return _elements; }
void reset() { _elements = 0; }
};

typedef WriteCheckpointEvent<ObjectSamplerStackTraceRepository> WriteObjectSamplerStackTrace;

static u4 flush_object_sampler_stacktrace(JfrStackTraceRepository& stack_trace_repo, ObjectSampler* sampler, JfrChunkWriter& chunkwriter) {
ObjectSamplerStackTraceRepository str(stack_trace_repo, sampler, chunkwriter, false);
WriteObjectSamplerStackTrace wst(chunkwriter, str, TYPE_STACKTRACE);
return invoke(wst);
}

static u4 write_object_sampler_stacktrace(JfrStackTraceRepository& stack_trace_repo, ObjectSampler* sampler, JfrChunkWriter& chunkwriter, bool clear) {
ObjectSamplerStackTraceRepository str(stack_trace_repo, sampler, chunkwriter, clear);
WriteObjectSamplerStackTrace wst(chunkwriter, str, TYPE_STACKTRACE);
return invoke(wst);
}

class StackTraceRepository : public StackObj {
private:
JfrStackTraceRepository& _repo;
Expand Down Expand Up @@ -380,6 +416,7 @@ JfrRecorderService::JfrRecorderService() :
_chunkwriter(JfrRepository::chunkwriter()),
_repository(JfrRepository::instance()),
_stack_trace_repository(JfrStackTraceRepository::instance()),
_leak_profiler_stack_trace_repository(JfrStackTraceRepository::leak_profiler_instance()),
_storage(JfrStorage::instance()),
_string_pool(JfrStringPool::instance()) {}

Expand Down Expand Up @@ -438,6 +475,9 @@ void JfrRecorderService::pre_safepoint_clear() {
_string_pool.clear();
_storage.clear();
_stack_trace_repository.clear();
if (LeakProfiler::is_running()) {
_leak_profiler_stack_trace_repository.clear();
}
}

void JfrRecorderService::invoke_safepoint_clear() {
Expand All @@ -453,6 +493,9 @@ void JfrRecorderService::safepoint_clear() {
_storage.clear();
_chunkwriter.set_time_stamp();
_stack_trace_repository.clear();
if (LeakProfiler::is_running()) {
_leak_profiler_stack_trace_repository.clear();
}
_checkpoint_manager.end_epoch_shift();
}

Expand Down Expand Up @@ -539,7 +582,7 @@ void JfrRecorderService::pre_safepoint_write() {
if (LeakProfiler::is_running()) {
// Exclusive access to the object sampler instance.
// The sampler is released (unlocked) later in post_safepoint_write.
ObjectSampleCheckpoint::on_rotation(ObjectSampler::acquire(), _stack_trace_repository);
ObjectSampleCheckpoint::on_rotation(ObjectSampler::acquire(), _leak_profiler_stack_trace_repository);
}
if (_string_pool.is_modified()) {
write_stringpool(_string_pool, _chunkwriter);
Expand All @@ -548,6 +591,11 @@ void JfrRecorderService::pre_safepoint_write() {
if (_stack_trace_repository.is_modified()) {
write_stacktrace(_stack_trace_repository, _chunkwriter, false);
}
if (LeakProfiler::is_running()) {
if (_leak_profiler_stack_trace_repository.is_modified()) {
write_object_sampler_stacktrace(_leak_profiler_stack_trace_repository, ObjectSampler::sampler(), _chunkwriter, false);
}
}
}

void JfrRecorderService::invoke_safepoint_write() {
Expand All @@ -567,6 +615,10 @@ void JfrRecorderService::safepoint_write() {
_storage.write_at_safepoint();
_chunkwriter.set_time_stamp();
write_stacktrace(_stack_trace_repository, _chunkwriter, true);
if (LeakProfiler::is_running()) {
// The object sampler instance was exclusively acquired and locked in pre_safepoint_write.
write_object_sampler_stacktrace(_leak_profiler_stack_trace_repository, ObjectSampler::sampler(), _chunkwriter, true);
}
_checkpoint_manager.end_epoch_shift();
}

Expand Down Expand Up @@ -624,6 +676,12 @@ size_t JfrRecorderService::flush() {
if (_stack_trace_repository.is_modified()) {
total_elements += flush_stacktrace(_stack_trace_repository, _chunkwriter);
}
if (LeakProfiler::is_running()) {
if (_leak_profiler_stack_trace_repository.is_modified()) {
total_elements += flush_object_sampler_stacktrace(_leak_profiler_stack_trace_repository, ObjectSampler::acquire(), _chunkwriter);
ObjectSampler::release();
}
}
return flush_typeset(_checkpoint_manager, _chunkwriter) + total_elements;
}

Expand Down
Expand Up @@ -40,6 +40,7 @@ class JfrRecorderService : public StackObj {
JfrChunkWriter& _chunkwriter;
JfrRepository& _repository;
JfrStackTraceRepository& _stack_trace_repository;
JfrStackTraceRepository& _leak_profiler_stack_trace_repository;
JfrStorage& _storage;
JfrStringPool& _string_pool;

Expand Down
Expand Up @@ -67,6 +67,7 @@ class JfrStackTrace : public JfrCHeapObj {
friend class ObjectSampleCheckpoint;
friend class ObjectSampler;
friend class OSThreadSampler;
friend class StackTraceChunkWriter;
friend class StackTraceResolver;
private:
const JfrStackTrace* _next;
Expand Down