From a811bc09689b0c031791b9dc39be06e68a92cd68 Mon Sep 17 00:00:00 2001 From: Thomas Schatzl Date: Mon, 26 Jan 2026 15:07:09 +0100 Subject: [PATCH] 8376335 Hi all, please review this change to make the `PreservedMarks` classes use `Atomic`. Some refactoring allowed removal of a now unnecessary function, and imo improved the code, avoiding the somewhat awkward passing of the atomic variable. Testing: gha Thanks, Thomas --- .../share/gc/shared/preservedMarks.cpp | 32 +++++++++---------- .../share/gc/shared/preservedMarks.hpp | 7 ++-- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/hotspot/share/gc/shared/preservedMarks.cpp b/src/hotspot/share/gc/shared/preservedMarks.cpp index 1c9f1c82e6f..1c9430d0950 100644 --- a/src/hotspot/share/gc/shared/preservedMarks.cpp +++ b/src/hotspot/share/gc/shared/preservedMarks.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2026, 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 @@ -29,15 +29,17 @@ #include "memory/allocation.inline.hpp" #include "memory/resourceArea.hpp" #include "oops/oop.inline.hpp" -#include "runtime/atomicAccess.hpp" +#include "runtime/atomic.hpp" #include "utilities/macros.hpp" -void PreservedMarks::restore() { +size_t PreservedMarks::restore() { + size_t result = size(); while (!_stack.is_empty()) { const PreservedMark elem = _stack.pop(); elem.set_mark(); } assert_empty(); + return result; } void PreservedMarks::adjust_preserved_mark(PreservedMark* elem) { @@ -55,15 +57,6 @@ void PreservedMarks::adjust_during_full_gc() { } } -void PreservedMarks::restore_and_increment(volatile size_t* const total_size_addr) { - const size_t stack_size = size(); - restore(); - // Only do the atomic add if the size is > 0. - if (stack_size > 0) { - AtomicAccess::add(total_size_addr, stack_size); - } -} - #ifndef PRODUCT void PreservedMarks::assert_empty() { assert(_stack.is_empty(), "stack expected to be empty, size = %zu", @@ -93,7 +86,7 @@ void PreservedMarksSet::init(uint num) { class RestorePreservedMarksTask : public WorkerTask { PreservedMarksSet* const _preserved_marks_set; SequentialSubTasksDone _sub_tasks; - volatile size_t _total_size; + Atomic _total_size; #ifdef ASSERT size_t _total_size_before; #endif // ASSERT @@ -102,7 +95,10 @@ public: void work(uint worker_id) override { uint task_id = 0; while (_sub_tasks.try_claim_task(task_id)) { - _preserved_marks_set->get(task_id)->restore_and_increment(&_total_size); + size_t num_restored = _preserved_marks_set->get(task_id)->restore(); + if (num_restored > 0) { + _total_size.add_then_fetch(num_restored); + } } } @@ -121,9 +117,11 @@ public: } ~RestorePreservedMarksTask() { - assert(_total_size == _total_size_before, "total_size = %zu before = %zu", _total_size, _total_size_before); - size_t mem_size = _total_size * (sizeof(oop) + sizeof(markWord)); - log_trace(gc)("Restored %zu marks, occupying %zu %s", _total_size, + size_t local_total_size = _total_size.load_relaxed(); + + assert(local_total_size == _total_size_before, "total_size = %zu before = %zu", local_total_size, _total_size_before); + size_t mem_size = local_total_size * (sizeof(oop) + sizeof(markWord)); + log_trace(gc)("Restored %zu marks, occupying %zu %s", local_total_size, byte_size_in_proper_unit(mem_size), proper_unit_for_byte_size(mem_size)); } diff --git a/src/hotspot/share/gc/shared/preservedMarks.hpp b/src/hotspot/share/gc/shared/preservedMarks.hpp index 10f75116524..af4d4abf1e7 100644 --- a/src/hotspot/share/gc/shared/preservedMarks.hpp +++ b/src/hotspot/share/gc/shared/preservedMarks.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2026, 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 @@ -61,7 +61,8 @@ public: inline void push_always(oop obj, markWord m); // Iterate over the stack, restore all preserved marks, and // reclaim the memory taken up by the stack segments. - void restore(); + // Returns the number of marks restored. + size_t restore(); // Adjust the preserved mark according to its // forwarding location stored in the mark. @@ -71,8 +72,6 @@ public: // to their forwarding location stored in the mark. void adjust_during_full_gc(); - void restore_and_increment(volatile size_t* const _total_size_addr); - // Assert the stack is empty and has no cached segments. void assert_empty() PRODUCT_RETURN;