From 955bfcd5502b3555c2c91db876be8e7535f2289a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Gr=C3=B6nlund?= Date: Wed, 4 Jun 2025 08:19:24 +0000 Subject: [PATCH] 8357671: JFR: Remove JfrTraceIdEpoch synchronizing Reviewed-by: egahlin --- .../checkpoint/jfrCheckpointManager.cpp | 9 ++----- .../checkpoint/jfrCheckpointManager.hpp | 3 +-- .../types/traceid/jfrTraceIdEpoch.cpp | 21 +++------------ .../types/traceid/jfrTraceIdEpoch.hpp | 26 +++++++++---------- .../recorder/service/jfrRecorderService.cpp | 6 ++--- .../jfr/recorder/stringpool/jfrStringPool.cpp | 5 +--- .../classes/jdk/jfr/internal/StringPool.java | 4 +-- 7 files changed, 23 insertions(+), 51 deletions(-) diff --git a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp index 7736d3f4565..b0f4461a82c 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp @@ -497,15 +497,10 @@ typedef CompositeOperation WriteRelease typedef VirtualThreadLocalCheckpointWriteOp VirtualThreadLocalCheckpointOperation; typedef MutexedWriteOp VirtualThreadLocalWriteOperation; -void JfrCheckpointManager::begin_epoch_shift() { - assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - JfrTraceIdEpoch::begin_epoch_shift(); -} - -void JfrCheckpointManager::end_epoch_shift() { +void JfrCheckpointManager::shift_epoch() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); DEBUG_ONLY(const u1 current_epoch = JfrTraceIdEpoch::current();) - JfrTraceIdEpoch::end_epoch_shift(); + JfrTraceIdEpoch::shift_epoch(); assert(current_epoch != JfrTraceIdEpoch::current(), "invariant"); JfrStringPool::on_epoch_shift(); } diff --git a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp index 53fa064e267..f9f8f1c26cf 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp @@ -96,8 +96,7 @@ class JfrCheckpointManager : public JfrCHeapObj { void clear_type_set(); void write_type_set(); - void begin_epoch_shift(); - void end_epoch_shift(); + void shift_epoch(); static void on_unloading_classes(); void on_rotation(); diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp index 8c2b8cdd0ec..a4ada594700 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp @@ -31,7 +31,7 @@ /* * The epoch generation is the range [1-32767]. * - * When the epoch value is stored in a thread object, + * When the epoch value is stored in a vthread object, * the most significant bit of the u2 is used to denote * thread exclusion, i.e 1 << 15 == 32768 denotes exclusion. */ @@ -39,32 +39,17 @@ u2 JfrTraceIdEpoch::_generation = 0; JfrSignal JfrTraceIdEpoch::_tag_state; bool JfrTraceIdEpoch::_method_tracer_state = false; bool JfrTraceIdEpoch::_epoch_state = false; -bool JfrTraceIdEpoch::_synchronizing = false; static constexpr const u2 epoch_generation_overflow = excluded_bit; -void JfrTraceIdEpoch::begin_epoch_shift() { +void JfrTraceIdEpoch::shift_epoch() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - _synchronizing = true; - OrderAccess::fence(); -} - -void JfrTraceIdEpoch::end_epoch_shift() { - assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - assert(_synchronizing, "invariant"); _epoch_state = !_epoch_state; - ++_generation; - if (epoch_generation_overflow == _generation) { + if (++_generation == epoch_generation_overflow) { _generation = 1; } assert(_generation != 0, "invariant"); assert(_generation < epoch_generation_overflow, "invariant"); - OrderAccess::storestore(); - _synchronizing = false; -} - -bool JfrTraceIdEpoch::is_synchronizing() { - return Atomic::load_acquire(&_synchronizing); } void JfrTraceIdEpoch::set_method_tracer_tag_state() { diff --git a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp index 10ea9643971..9e2d2f0708a 100644 --- a/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp +++ b/src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp @@ -26,7 +26,6 @@ #define SHARE_JFR_RECORDER_CHECKPOINT_TYPES_TRACEID_JFRTRACEIDEPOCH_HPP #include "jfr/utilities/jfrSignal.hpp" -#include "jfr/utilities/jfrTypes.hpp" #include "memory/allStatic.hpp" #define BIT 1 @@ -41,16 +40,17 @@ #define EPOCH_0_METHOD_AND_CLASS_BITS (METHOD_AND_CLASS_BITS << EPOCH_0_SHIFT) #define EPOCH_1_METHOD_AND_CLASS_BITS (METHOD_AND_CLASS_BITS << EPOCH_1_SHIFT) - // Epoch alternation on each rotation allow for concurrent tagging. - // The epoch shift happens only during a safepoint. - // - // _synchronizing is a transition state, the purpose of which is to - // have JavaThreads that run _thread_in_native (i.e. Compiler threads) - // respect the current epoch shift in-progress during the safepoint. - // - // _changed_tag_state == true signals an incremental modification to artifact tagging - // (klasses, methods, CLDs, etc), purpose of which is to trigger collection of artifacts. - // +/* + * An epoch shift or alternation on each rotation enables concurrent tagging. + * The epoch shift happens only during a safepoint. + * + * _generation - mainly used with virtual threads, but also for the generational string pool in Java. + * _tag_state - signals an incremental modification to artifact tagging (klasses, methods, CLDs, etc) + * purpose of which is to trigger a collection of artifacts. + * _method_tracer_state - a special notification state only used with method timing and tracing. + * _epoch_state - the fundamental binary epoch state that shifts on each rotation during a safepoint. + */ + class JfrTraceIdEpoch : AllStatic { friend class JfrCheckpointManager; private: @@ -58,10 +58,8 @@ class JfrTraceIdEpoch : AllStatic { static JfrSignal _tag_state; static bool _method_tracer_state; static bool _epoch_state; - static bool _synchronizing; - static void begin_epoch_shift(); - static void end_epoch_shift(); + static void shift_epoch(); public: static bool epoch() { diff --git a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp index 07fe019c9af..7d1d7ac0a05 100644 --- a/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp +++ b/src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp @@ -484,13 +484,12 @@ void JfrRecorderService::invoke_safepoint_clear() { void JfrRecorderService::safepoint_clear() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - _checkpoint_manager.begin_epoch_shift(); _storage.clear(); _checkpoint_manager.notify_threads(); _chunkwriter.set_time_stamp(); JfrDeprecationManager::on_safepoint_clear(); JfrStackTraceRepository::clear(); - _checkpoint_manager.end_epoch_shift(); + _checkpoint_manager.shift_epoch(); } void JfrRecorderService::post_safepoint_clear() { @@ -593,14 +592,13 @@ void JfrRecorderService::invoke_safepoint_write() { void JfrRecorderService::safepoint_write() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - _checkpoint_manager.begin_epoch_shift(); JfrStackTraceRepository::clear_leak_profiler(); _checkpoint_manager.on_rotation(); _storage.write_at_safepoint(); _chunkwriter.set_time_stamp(); JfrDeprecationManager::on_safepoint_write(); write_stacktrace(_stack_trace_repository, _chunkwriter, true); - _checkpoint_manager.end_epoch_shift(); + _checkpoint_manager.shift_epoch(); } void JfrRecorderService::post_safepoint_write() { diff --git a/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp b/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp index a9e39094f79..dc28818b0f9 100644 --- a/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp +++ b/src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp @@ -44,8 +44,6 @@ static int generation_offset = invalid_offset; static jobject string_pool = nullptr; -static unsigned short generation = 0; - static bool setup_string_pool_offsets(TRAPS) { const char class_name[] = "jdk/jfr/internal/StringPool"; Symbol* const k_sym = SymbolTable::new_symbol(class_name); @@ -281,9 +279,8 @@ void JfrStringPool::register_full(BufferPtr buffer, Thread* thread) { void JfrStringPool::on_epoch_shift() { assert(SafepointSynchronize::is_at_safepoint(), "invariant"); - assert(!JfrTraceIdEpoch::is_synchronizing(), "invariant"); assert(string_pool != nullptr, "invariant"); oop mirror = JfrJavaSupport::resolve_non_null(string_pool); assert(mirror != nullptr, "invariant"); - mirror->short_field_put(generation_offset, generation++); + mirror->short_field_put(generation_offset, JfrTraceIdEpoch::epoch_generation()); } diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java index c0dc4e6ba58..41e8a48cfe6 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/StringPool.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2025, 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 @@ -51,7 +51,7 @@ public final class StringPool { private static int preCacheOld = 0; /* max size bytes */ private static long currentSizeUTF16; - /* string pool generation (0-65535) set by the JVM on epoch shift. Not private to avoid being optimized away. */ + /* The string pool epoch generation is the range [1-32767] set by the JVM on epoch shift. Not private to avoid being optimized away. */ static short generation = 0; /* internalSid is a composite id [48-bit externalSid][16-bit generation]. */