RFR: 8372250: Merge PtrQueue into SATBMarkQueue
Kim Barrett
kbarrett at openjdk.org
Wed Dec 10 21:45:33 UTC 2025
On Wed, 10 Dec 2025 18:26:16 GMT, Ben Taylor <btaylor at openjdk.org> wrote:
> This change merges `PtrQueue` into its only child class, `SATBMarkQueue`. The corresponding `PtrQueueSet` is merged with `SATBMarkQueueSet`, and `G1SATBMarkQueueSet` and `ShenandoahSATBMarkQueueSet` continue to inherit from that class.
>
> There are few code modifications - most methods were moved directly from `PtrQueue` to `SATBMarkQueue`. The one meaningful change is to preserve `SATBMarkQueue`'s call to `filter(queue);` before `flush_queue`.
>
> This change passes all tier1 jtreg tests with both Shenandoah and G1 in local testing.
This looks mostly good, with just a few minor nits.
I think there may be further simplifications and cleanups possible, but
looking for those can be done later, once this relatively clean merging change
is done.
[appropriate place is not commentable in github diff]
SATBMarkQueueSet::enqueue_completed_buffer should no longer be virtual. (This
would have been caught if it had been changed to "override" once that was
permitted, but nobody got around to that.)
src/hotspot/share/gc/shared/satbMarkQueue.cpp line 50:
> 48:
> 49: SATBMarkQueue::~SATBMarkQueue() {
> 50: assert(_buf == nullptr, "queue must be flushed before delete");
Too much indentation.
src/hotspot/share/gc/shared/satbMarkQueue.cpp line 428:
> 426: void SATBMarkQueueSet::deallocate_buffer(BufferNode* node) {
> 427: _allocator->release(node);
> 428: }
Missing newline?
src/hotspot/share/gc/shared/satbMarkQueue.hpp line 196:
> 194: // Installs a new buffer into queue.
> 195: void install_new_buffer(SATBMarkQueue& queue);
> 196:
These helper functions hoisted from PtrQueueSet should be private rather than protected here.
src/hotspot/share/gc/shared/satbMarkQueue.hpp line 249:
> 247:
> 248: // A completed buffer is a buffer the mutator is finished with, and
> 249: // is ready to be processed by the collector. It need not be full.
This comment (copied from ptrQueueSet) seems misplaced here. Perhaps it should be part of the
class description?
-------------
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/28755#pullrequestreview-3564478203
PR Review Comment: https://git.openjdk.org/jdk/pull/28755#discussion_r2608208876
PR Review Comment: https://git.openjdk.org/jdk/pull/28755#discussion_r2608213644
PR Review Comment: https://git.openjdk.org/jdk/pull/28755#discussion_r2608282954
PR Review Comment: https://git.openjdk.org/jdk/pull/28755#discussion_r2608246180
More information about the hotspot-gc-dev
mailing list