RFR: 8373595: A new ObjectMonitorTable implementation [v3]

[Fredrik Bredberg]

On Tue, 27 Jan 2026 06:48:59 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Fredrik Bredberg has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updated shared code after review comments
>
> src/hotspot/share/runtime/lockStack.hpp line 142:
> 
>> 140:     ObjectMonitor* _monitor = nullptr;
>> 141:   } _entries[CAPACITY];
>> 142:   const oop _null_sentinel = nullptr;
> 
> This should no longer be needed. As we no longer have any loops in C2 that terminates on a null read.

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 51:
> 
>> 49:   char _padding[DEFAULT_CACHE_LINE_SIZE];
>> 50: 
>> 51:   volatile size_t _items_count;
> 
> I wonder if we want something along the lines of this here. We do not allocate many of these tables, so using some extra padding to make sure we never share cache lines with anything outside the objects either (even though it will probably always just be our NMT headers).
> Suggestion:
> 
>   DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE, 0);
>   const size_t _capacity_mask;       // One less than its power-of-two capacity
>   Table* volatile _prev;             // Set while rehashing
>   ObjectMonitor* volatile* _buckets; // The payload
> 
>   DEFINE_PAD_MINUS_SIZE(1, DEFAULT_CACHE_LINE_SIZE, sizeof(_capacity_mask) + sizeof(_prev) + sizeof(_buckets));
>   volatile size_t _items_count;
> 
>   DEFINE_PAD_MINUS_SIZE(2, DEFAULT_CACHE_LINE_SIZE, sizeof(_items_count));
> 
> 
> Once we add alignment support for our CHeapObj operator new we can replace this with:
> ```c++
>   const size_t _capacity_mask;       // One less than its power-of-two capacity
>   Table* volatile _prev;             // Set while rehashing
>   ObjectMonitor* volatile* _buckets; // The payload
> 
>   alignas(DEFAULT_CACHE_LINE_SIZE) volatile size_t _items_count;

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 118:
> 
>> 116:   }
>> 117: 
>> 118:   ObjectMonitor* get(oop obj, int hash) {
> 
> While it might be wrong that we type the hash() interface on the markWord as `intptr_t`. I think we should stick with that type until we decide to change this in the markWord inteface. When I see a `intptr_t` -> `int` -> `size_t` roundtrip I try and figure out what the purpose is.

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 153:
> 
>> 151:     }
>> 152: 
>> 153:     // Rehashing could have stareted by now, but if a monitor has been inserted in a
> 
> Suggestion:
> 
>     // Rehashing could have started by now, but if a monitor has been inserted in a

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 218:
> 
>> 216:   }
>> 217: 
>> 218:   void remove(oop obj, ObjectMonitor* old_monitor, int hash) {
> 
> Same type comment. (`int` -> `intptr_t`)

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 252:
> 
>> 250: 
>> 251:   void reinsert(oop obj, ObjectMonitor* new_monitor) {
>> 252:     int hash = obj->mark().hash();
> 
> Same type comment. (`int` -> `intptr_t`)

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 334:
> 
>> 332:         // A monitor
>> 333:         oop obj = monitor->object_peek();
>> 334:         if (!monitor->is_being_async_deflated() && obj != nullptr) {
> 
> While there is nothing inherently wrong with the check. In the current implementation will we ever encounter a `monitor->is_being_async_deflated() == true`.
> 
> As it is the deflation thread which drives the rebuilding, and it will already have removed any entry it has deflated.

Rewrote, added an assert().

> src/hotspot/share/runtime/objectMonitorTable.cpp line 351:
> 
>> 349: 
>> 350: ObjectMonitor* ObjectMonitorTable::monitor_get(Thread* current, oop obj) {
>> 351:   const int hash = obj->mark().hash();
> 
> Same type comment. (`int` -> `intptr_t`)

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 382:
> 
>> 380: 
>> 381: ObjectMonitor* ObjectMonitorTable::monitor_put_get(Thread* current, ObjectMonitor* monitor, oop obj) {
>> 382:   const int hash = obj->mark().hash();
> 
> Same type comment. (`int` -> `intptr_t`)

Fixed

> src/hotspot/share/runtime/objectMonitorTable.cpp line 402:
> 
>> 400:     return;
>> 401:   }
>> 402:   const int hash = obj->mark().hash();
> 
> Same type comment. (`int` -> `intptr_t`)

Fixed

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788041898
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788043914
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788059930
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788064697
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788061002
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788053470
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788068712
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788070517
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788054430
PR Review Comment: https://git.openjdk.org/jdk/pull/29383#discussion_r2788063890