RFR: 8256302: releasing oopStorage when deflating allows for faster deleting

[Daniel D. Daugherty]

On Fri, 2 Dec 2022 02:11:49 GMT, David Holmes <dholmes at openjdk.org> wrote:

>>> I still think we should have been able to keep the release in the destructor guarded by some check:
>>> 
>>> ```
>>> if (! already_released) {
>>>   assert(!safepoint_safe, "must be");
>>>   _object.release(_oop_storage);
>>> }
>>> ```
>>> 
>>> but perhaps there is no way to ask if "already_released"?
>>> 
>>> Thanks for the additional comments on the why of all this. The constraints on when you can access oopStorage were not clear before.
>> 
>> You can ask by doing _object().is_null(), if true there is no backing storage in oopStorage.
>> Since the case above should never ever happen (cross-fingers) I would:
>> 
>> `guarantee(_object().is_null(), "Destructor should never be called if weak handle is not released");`
>
>> You can ask by doing `_object().is_null()`
> 
> Are you allowed to do that when safepoint-safe?

I tried to figure out a way to reply to @dholmes-ora comment above in the context of
that comment, but I couldn't find the magic sequence...

> I'm finding it a bit hard to follow the logic here and I think some older PR comments may be causing me some confusion.

Sorry. I tried to add clarifications to the older comments where I thought that they
needed it. I think I added to just one, but maybe that wasn't enough...

 
> IIUC the key enhancement here is that when we deflate we will also release the
> oop_storage, and in doing so we mark the `_object` as null so that the destructor
> skips also trying to do a release.

The above sentence is correct.


> Otherwise the destructor of OM will take care of the release when we `deflate_idle_monitors`.

Hopefully you meant "when we directly delete the ObjectMonitor".  There are a couple of places
where we do a direct delete of an ObjectMonitor that is no longer needed. From those call sites,
we will end up calling `release_object()` from the ObjectMonitor destructor. In the previous version
of the fix, I had a direct call to `release_object()` from those direct delete call sites that folks
thought was "clunky" so now it is gone.

Of course, you might mean something else because I'm really just guessing here... 

> In the latter case we make the current JavaThread `_thread_blocked` so it doesn't delay safepoints. Is that all correct?

When we are deflating idle ObjectMonitors, we call `release_object()` when we deflate,
we make the current JavaThread `_thread_blocked` AND lastly we do the mass delete
of the ObjectMonitors that we deflated. It's that last part (the mass delete) that we want
to do while blocked so we don't delay safepoints.

> Thanks.

Thanks for hanging in there on this RFE's long review and development cycle.

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

PR Comment: https://git.openjdk.org/jdk/pull/11296#issuecomment-1577384462