--- old/src/hotspot/share/runtime/heapMonitoring.cpp 2018-03-08 15:53:31.947587202 -0800 +++ new/src/hotspot/share/runtime/heapMonitoring.cpp 2018-03-08 15:53:31.683588094 -0800 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, Google and/or its affiliates. All rights reserved. + * Copyright (c) 2018, Google 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 @@ -221,7 +221,8 @@ public: // The function that gets called to add a trace to the list of // traces we are maintaining. - void add_trace(jvmtiAllocTraceInfo* trace, oop o); + // Returns if the trace got added or not. + bool add_trace(jvmtiAllocTraceInfo* trace, oop o); // The function that gets called by the client to retrieve the list // of stack traces. Passes a jvmtiAllocTraceInfo which will get mutated. @@ -280,11 +281,6 @@ _stats.sample_rate_count++; } - bool initialized() { - return OrderAccess::load_acquire(&_initialized) != 0; - return _initialized; - } - private: // The traces currently sampled. GrowableArray* _allocated_traces; @@ -305,7 +301,7 @@ int _max_gc_storage; static StackTraceStorage* internal_storage; - int _initialized; + bool _initialized; // Support functions and classes for copying data to the external // world. @@ -392,7 +388,7 @@ _recent_garbage_traces = NULL; _frequent_garbage_traces = NULL; _max_gc_storage = 0; - OrderAccess::release_store(&_initialized, 0); + _initialized = false; } void StackTraceStorage::free_garbage() { @@ -425,7 +421,7 @@ } void StackTraceStorage::free_storage() { - if (!initialized()) { + if (!_initialized) { return; } @@ -440,7 +436,6 @@ } StackTraceStorage::~StackTraceStorage() { - MutexLocker mu(HeapMonitorStorage_lock); free_storage(); } @@ -450,7 +445,7 @@ "This should not be accessed concurrently"); // In case multiple threads got locked and then 1 by 1 got through. - if (initialized()) { + if (_initialized) { return; } @@ -464,26 +459,37 @@ _max_gc_storage = max_gc_storage; memset(&_stats, 0, sizeof(_stats)); - OrderAccess::release_store(&_initialized, 1); + _initialized = true; } -void StackTraceStorage::add_trace(jvmtiAllocTraceInfo* trace, oop o) { +bool StackTraceStorage::add_trace(jvmtiAllocTraceInfo* trace, oop o) { MutexLocker mu(HeapMonitorStorage_lock); - // Last minute check on initialization here in case: - // Between the moment object_alloc_do_sample's check for initialization - // and now, there was a stop() that deleted the data. - if (initialized()) { - StackTraceDataWithOop new_data(trace, o); - _stats.sample_count++; - _stats.stack_depth_accumulation += trace->stack_info->frame_count; - _allocated_traces->append(new_data); + // Last minute check on initialization here in case the system got turned off + // and a few sample requests got through to here, ie: + // A sample point happened in TLAB, that code checks for + // HeapMonitoring::enabled and calls object_alloc_do_sample. + // The code starts getting a stacktrace and then calls the this add_trace. + // + // At the same time, another thread has turned off HeapMonitoring while the + // stacktraces were getting constructed and disables StackTraceStorage. + // + // Both disabling and this add_trace are protected by the same + // HeapMonitorStorage_lock mutex. + if (!_initialized) { + return false; } + + StackTraceDataWithOop new_data(trace, o); + _stats.sample_count++; + _stats.stack_depth_accumulation += trace->stack_info->frame_count; + _allocated_traces->append(new_data); + return true; } void StackTraceStorage::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* f) { size_t count = 0; - if (initialized()) { + if (_initialized) { int len = _allocated_traces->length(); _traces_on_last_full_gc->clear(); @@ -849,59 +855,56 @@ void HeapMonitoring::object_alloc_do_sample(Thread* t, oopDesc* o, size_t byte_size) { JavaThread* thread = static_cast(t); - if (StackTraceStorage::storage()->initialized()) { - assert(t->is_Java_thread(), "non-Java thread passed to do_sample"); - JavaThread* thread = static_cast(t); + assert(t->is_Java_thread(), "non-Java thread passed to do_sample"); - jvmtiAllocTraceInfo* trace = NEW_C_HEAP_OBJ(jvmtiAllocTraceInfo, mtInternal); - if (trace == NULL) { - return; - } + jvmtiAllocTraceInfo* trace = NEW_C_HEAP_OBJ(jvmtiAllocTraceInfo, mtInternal); + if (trace == NULL) { + return; + } - jvmtiStackInfo* stack_info = NEW_C_HEAP_OBJ(jvmtiStackInfo, mtInternal); - if (trace == NULL) { - FREE_C_HEAP_OBJ(trace); - return; - } - trace->stack_info = stack_info; + jvmtiStackInfo* stack_info = NEW_C_HEAP_OBJ(jvmtiStackInfo, mtInternal); + if (trace == NULL) { + FREE_C_HEAP_OBJ(trace); + return; + } + trace->stack_info = stack_info; - jvmtiFrameInfo* frames = - NEW_C_HEAP_ARRAY(jvmtiFrameInfo, MaxStackDepth, mtInternal); + jvmtiFrameInfo* frames = + NEW_C_HEAP_ARRAY(jvmtiFrameInfo, MaxStackDepth, mtInternal); - if (frames == NULL) { - FREE_C_HEAP_OBJ(stack_info); - FREE_C_HEAP_OBJ(trace); - return; - } - stack_info->frame_buffer = frames; - stack_info->frame_count = 0; + if (frames == NULL) { + FREE_C_HEAP_OBJ(stack_info); + FREE_C_HEAP_OBJ(trace); + return; + } + stack_info->frame_buffer = frames; + stack_info->frame_count = 0; - trace->thread_id = SharedRuntime::get_java_tid(thread); - trace->size = byte_size; + trace->thread_id = SharedRuntime::get_java_tid(thread); + trace->size = byte_size; - if (thread->has_last_Java_frame()) { // just to be safe - vframeStream vfst(thread, true); - int count = 0; - while (!vfst.at_end() && count < MaxStackDepth) { - Method* m = vfst.method(); - frames[count].location = vfst.bci(); - frames[count].method = m->jmethod_id(); - count++; + if (thread->has_last_Java_frame()) { // just to be safe + vframeStream vfst(thread, true); + int count = 0; + while (!vfst.at_end() && count < MaxStackDepth) { + Method* m = vfst.method(); + frames[count].location = vfst.bci(); + frames[count].method = m->jmethod_id(); + count++; - vfst.next(); - } - stack_info->frame_count = count; + vfst.next(); } + stack_info->frame_count = count; + } - if (stack_info->frame_count > 0) { - // Success! - StackTraceStorage::storage()->add_trace(trace, o); + if (stack_info->frame_count > 0) { + if (StackTraceStorage::storage()->add_trace(trace, o)) { return; } - - // Failure! - FREE_C_HEAP_ARRAY(jvmtiFrameInfo, frames); - FREE_C_HEAP_OBJ(stack_info); - FREE_C_HEAP_OBJ(trace); } + + // Failure! + FREE_C_HEAP_ARRAY(jvmtiFrameInfo, frames); + FREE_C_HEAP_OBJ(stack_info); + FREE_C_HEAP_OBJ(trace); }