On Tue, 23 Jul 2024 16:36:18 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 102: >> >>> 100: assert(*value != nullptr, "must be"); >>> 101: return (*value)->object_is_cleared(); >>> 102: } >> >> The `is_dead` functions seem oddly placed given they do not relate to the >> object stored in the wrapper. Why are they here? And what is the difference >> between `object_is_cleared` and `object_is_dead` (as used by >> `LookupMonitor`) ? > > This is a good question. When we look up the Monitor, we don't want to find > any that the GC has marked dead, so that's why we call object_is_dead. When > we look up with the object to find the Monitor, the object won't be dead > (since we're using it to look up). But we don't want to find one that we've > cleared because the Monitor was deflated? I don't see where we would clear > it though. We clear the WeakHandle in the destructor after the Monitor has > been removed from the table. What @coleenp said is correct. One is just a null check, the other interacts with the GC and the objects lifecycle. But as also mentioned we do not ever clear these any WeakHandle, so this is currently always returning false. (At least from what I can see). This load should be cached or in a register because it always load this to check if it is the value when doing a lookup. So there should be no performance cost here, but it is unnecessary. I'll remove this. The `object_is_dead` is only used when removing, where we can take the extra cost. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715347334
