From ac9ca9719950ca31bd24faa9f22ebb825aed1df4 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Mon, 22 Apr 2024 07:14:15 +0000 Subject: [PATCH 1/3] 8329528: G1 does not update TAMS correctly when dropping retained regions during Concurrent Start pause Reviewed-by: ayang, chegar --- src/hotspot/share/gc/g1/g1YoungCollector.cpp | 13 ++-- .../share/gc/g1/g1YoungGCPreEvacuateTasks.hpp | 1 + src/hotspot/share/gc/g1/heapRegion.inline.hpp | 2 +- .../pinnedobjs/TestDroppedRetainedTAMS.java | 72 +++++++++++++++++++ 4 files changed, 80 insertions(+), 8 deletions(-) create mode 100644 test/hotspot/jtreg/gc/g1/pinnedobjs/TestDroppedRetainedTAMS.java diff --git a/src/hotspot/share/gc/g1/g1YoungCollector.cpp b/src/hotspot/share/gc/g1/g1YoungCollector.cpp index 4fa3b928763..906d9854d8d 100644 --- a/src/hotspot/share/gc/g1/g1YoungCollector.cpp +++ b/src/hotspot/share/gc/g1/g1YoungCollector.cpp @@ -484,13 +484,8 @@ void G1YoungCollector::set_young_collection_default_active_worker_threads(){ } void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info) { - - // Must be before collection set calculation, requires collection set to not - // be calculated yet. - if (collector_state()->in_concurrent_start_gc()) { - concurrent_mark()->pre_concurrent_start(_gc_cause); - } - + // Flush various data in thread-local buffers to be able to determine the collection + // set { Ticks start = Ticks::now(); G1PreEvacuateCollectionSetBatchTask cl; @@ -501,6 +496,10 @@ void G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo* evacuation_info) // Needs log buffers flushed. calculate_collection_set(evacuation_info, policy()->max_pause_time_ms()); + if (collector_state()->in_concurrent_start_gc()) { + concurrent_mark()->pre_concurrent_start(_gc_cause); + } + // Please see comment in g1CollectedHeap.hpp and // G1CollectedHeap::ref_processing_init() to see how // reference processing currently works in G1. diff --git a/src/hotspot/share/gc/g1/g1YoungGCPreEvacuateTasks.hpp b/src/hotspot/share/gc/g1/g1YoungGCPreEvacuateTasks.hpp index effefa7a1eb..912941fa2a2 100644 --- a/src/hotspot/share/gc/g1/g1YoungGCPreEvacuateTasks.hpp +++ b/src/hotspot/share/gc/g1/g1YoungGCPreEvacuateTasks.hpp @@ -29,6 +29,7 @@ // Set of pre evacuate collection set tasks containing ("s" means serial): // - Retire TLAB and Flush Logs (Java threads) +// - Flush pin count cache (Java threads) // - Flush Logs (s) (Non-Java threads) class G1PreEvacuateCollectionSetBatchTask : public G1BatchedTask { class JavaThreadRetireTLABAndFlushLogs; diff --git a/src/hotspot/share/gc/g1/heapRegion.inline.hpp b/src/hotspot/share/gc/g1/heapRegion.inline.hpp index 608cc2025a2..f126721f6cb 100644 --- a/src/hotspot/share/gc/g1/heapRegion.inline.hpp +++ b/src/hotspot/share/gc/g1/heapRegion.inline.hpp @@ -289,7 +289,7 @@ inline void HeapRegion::reset_parsable_bottom() { inline void HeapRegion::note_start_of_marking() { assert(top_at_mark_start() == bottom(), "Region's TAMS must always be at bottom"); - if (is_old_or_humongous() && !is_collection_set_candidate()) { + if (is_old_or_humongous() && !is_collection_set_candidate() && !in_collection_set()) { set_top_at_mark_start(top()); } } diff --git a/test/hotspot/jtreg/gc/g1/pinnedobjs/TestDroppedRetainedTAMS.java b/test/hotspot/jtreg/gc/g1/pinnedobjs/TestDroppedRetainedTAMS.java new file mode 100644 index 00000000000..f650e53a25f --- /dev/null +++ b/test/hotspot/jtreg/gc/g1/pinnedobjs/TestDroppedRetainedTAMS.java @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2024, 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 + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/* @test + * @summary Check that TAMSes are correctly updated for regions dropped from + * the retained collection set candidates during a Concurrent Start pause. + * @requires vm.gc.G1 + * @requires vm.flagless + * @library /test/lib + * @build jdk.test.whitebox.WhiteBox + * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox + * @run main/othervm -XX:+UseG1GC -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions + -XX:+WhiteBoxAPI -Xbootclasspath/a:. -Xmx32m -XX:G1NumCollectionsKeepPinned=1 + -XX:+VerifyBeforeGC -XX:+VerifyAfterGC -XX:G1MixedGCLiveThresholdPercent=100 + -XX:G1HeapWastePercent=0 -Xlog:gc,gc+ergo+cset=trace gc.g1.pinnedobjs.TestDroppedRetainedTAMS + */ + +package gc.g1.pinnedobjs; + +import jdk.test.whitebox.WhiteBox; + +public class TestDroppedRetainedTAMS { + + private static final WhiteBox wb = WhiteBox.getWhiteBox(); + + private static final char[] dummy = new char[100]; + + public static void main(String[] args) { + wb.fullGC(); // Move the target dummy object to old gen. + + wb.pinObject(dummy); + + // After this concurrent cycle the pinned region will be in the the (marking) + // collection set candidates. + wb.g1RunConcurrentGC(); + + // Pass the Prepare mixed gc which will not do anything about the marking + // candidates. + wb.youngGC(); + // Mixed GC. Will complete. That pinned region is now retained. The mixed gcs + // will end here. + wb.youngGC(); + + // The pinned region will be dropped from the retained candidates during the + // Concurrent Start GC, leaving that region's TAMS broken. + wb.g1RunConcurrentGC(); + + // Verification will find a lot of broken objects. + wb.youngGC(); + System.out.println(dummy); + } +} From ccb1a3ed5f2cf85aa086007a2f60609d9ec81227 Mon Sep 17 00:00:00 2001 From: Sean Coffey Date: Mon, 22 Apr 2024 07:38:59 +0000 Subject: [PATCH 2/3] 8324933: ConcurrentHashTable::statistics_calculate synchronization is expensive Backport-of: 0e2fdc95ae47c11e6a1e47cdc6190268e29a9d9c --- .../utilities/concurrentHashTable.inline.hpp | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp index ee9e7c4bfc8..5fab7917235 100644 --- a/src/hotspot/share/utilities/concurrentHashTable.inline.hpp +++ b/src/hotspot/share/utilities/concurrentHashTable.inline.hpp @@ -1223,23 +1223,30 @@ template inline TableStatistics ConcurrentHashTable:: statistics_calculate(Thread* thread, VALUE_SIZE_FUNC& vs_f) { + constexpr size_t batch_size = 128; NumberSeq summary; size_t literal_bytes = 0; InternalTable* table = get_table(); - for (size_t bucket_it = 0; bucket_it < table->_size; bucket_it++) { + size_t num_batches = table->_size / batch_size; + for (size_t batch_start = 0; batch_start < _table->_size; batch_start += batch_size) { + // We batch the use of ScopedCS here as it has been found to be quite expensive to + // invoke it for every single bucket. + size_t batch_end = MIN2(batch_start + batch_size, _table->_size); ScopedCS cs(thread, this); - size_t count = 0; - Bucket* bucket = table->get_bucket(bucket_it); - if (bucket->have_redirect() || bucket->is_locked()) { - continue; - } - Node* current_node = bucket->first(); - while (current_node != nullptr) { - ++count; - literal_bytes += vs_f(current_node->value()); - current_node = current_node->next(); + for (size_t bucket_it = batch_start; bucket_it < batch_end; bucket_it++) { + size_t count = 0; + Bucket* bucket = table->get_bucket(bucket_it); + if (bucket->have_redirect() || bucket->is_locked()) { + continue; + } + Node* current_node = bucket->first(); + while (current_node != nullptr) { + ++count; + literal_bytes += vs_f(current_node->value()); + current_node = current_node->next(); + } + summary.add((double)count); } - summary.add((double)count); } if (_stats_rate == nullptr) { From 5f333b578d5f614d7ffa5878de09debeebf27fbc Mon Sep 17 00:00:00 2001 From: Sean Coffey Date: Mon, 22 Apr 2024 07:39:24 +0000 Subject: [PATCH 3/3] 8326106: Write and clear stack trace table outside of safepoint Backport-of: a776104e210db212c4e32894844d3c0cbaac53c3 --- .../checkpoint/objectSampleCheckpoint.cpp | 16 ++++++++-------- .../jfr/recorder/service/jfrRecorderService.cpp | 4 +--- .../stacktrace/jfrStackTraceRepository.cpp | 3 +-- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp b/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp index 9f6679c93eb..ba0a53cb4b8 100644 --- a/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp +++ b/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleCheckpoint.cpp @@ -214,9 +214,6 @@ class StackTraceBlobInstaller { StackTraceBlobInstaller() : _cache(JfrOptionSet::old_object_queue_size()) { prepare_for_resolution(); } - ~StackTraceBlobInstaller() { - JfrStackTraceRepository::clear_leak_profiler(); - } void sample_do(ObjectSample* sample) { if (stack_trace_precondition(sample)) { install(sample); @@ -270,11 +267,14 @@ void ObjectSampleCheckpoint::on_rotation(const ObjectSampler* sampler) { assert(LeakProfiler::is_running(), "invariant"); JavaThread* const thread = JavaThread::current(); DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(thread);) - // can safepoint here - ThreadInVMfromNative transition(thread); - MutexLocker lock(ClassLoaderDataGraph_lock); - // the lock is needed to ensure the unload lists do not grow in the middle of inspection. - install_stack_traces(sampler); + { + // can safepoint here + ThreadInVMfromNative transition(thread); + MutexLocker lock(ClassLoaderDataGraph_lock); + // the lock is needed to ensure the unload lists do not grow in the middle of inspection. + install_stack_traces(sampler); + } + JfrStackTraceRepository::clear_leak_profiler(); } static bool is_klass_unloaded(traceid klass_id) { diff --git a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp index fe674bc9610..008c402ef48 100644 --- a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp +++ b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp @@ -573,9 +573,7 @@ void JfrRecorderService::pre_safepoint_write() { ObjectSampleCheckpoint::on_rotation(ObjectSampler::acquire()); } write_storage(_storage, _chunkwriter); - if (_stack_trace_repository.is_modified()) { - write_stacktrace(_stack_trace_repository, _chunkwriter, false); - } + write_stacktrace(_stack_trace_repository, _chunkwriter, true); } void JfrRecorderService::invoke_safepoint_write() { diff --git a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp index 3cc93545097..a1d9f6efb19 100644 --- a/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp +++ b/src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp @@ -98,11 +98,10 @@ bool JfrStackTraceRepository::is_modified() const { } size_t JfrStackTraceRepository::write(JfrChunkWriter& sw, bool clear) { + MutexLocker lock(JfrStacktrace_lock, Mutex::_no_safepoint_check_flag); if (_entries == 0) { return 0; } - MutexLocker lock(JfrStacktrace_lock, Mutex::_no_safepoint_check_flag); - assert(_entries > 0, "invariant"); int count = 0; for (u4 i = 0; i < TABLE_SIZE; ++i) { JfrStackTrace* stacktrace = _table[i];