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

[Daniel D. Daugherty]

On Wed, 23 Nov 2022 00:33:54 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/runtime/synchronizer.cpp line 1266:
>> 
>>> 1264:       if (cmp != mark) {
>>> 1265:         // Release object's oop storage since we don't need this ObjectMonitor:
>>> 1266:         m->release_object();
>> 
>> This looks a bit clunky - the caller should not need to know about the ObjectMonitor internals with regards to Oop storage management. Can't we keep the release in the destructor but guard it (if necessary) with a check to see if it is already released? It would be nice if release could be called more than once and was a no-op if already released. (I can't make enough sense of the Access API to know whether this is feasible/possible).
>
> This also made me wonder ... are we guaranteed the current thread is a JavaThread? If so the signature and use of `ObjectSynchronizer::inflate(Thread* current` could be changed. If not, isn't that a requirements for oop storage changes?

@dholmes-ora - It is indeed clunky. I originally had the release in the destructor
but I could not find a way to make that work reliably. IIRC, I saw intermittent crashes
when I tested that version. I'll have to check the history of my patches for this fix,
but I think I added a check in the destructor to see if the oop needed to be released,
did the release if needed and then cleared the oop. Something like that anyway,
but it didn't work. (Update: @rehn's comments reminded me why it didn't work).
Also ObjectSynchronizer knows quite a bit about ObjectMonitor internals so I think
it's okay for inflate() to know about release oop storage.

As for the caller being a JavaThread*, I'm not sure if we have any non-JavaThreads
inflating ObjectMonitors any more. I know we allowed this in the past, but I'm not
sure if it is allowed now. I also don't know if Oop Storage changes require a JavaThread
or not. I'll have to ask @fisk.

@robehn - I'll check into adding that assert that WeakHandle is null.

@coleenp -
> Isn't the thread in the vm?

The JavaThread was in the VM which is why we added the `ThreadBlockInVM` to get
it out of the VM and into blocked. But the code you're quoting is not in the
function we're discussing here. It's down in `deflate_idle_monitors`.

> why the release is moved out of the destructor also.

I moved it out of the because I couldn't get it to work reliably when I did
the release in the destructor. @robehn's comments reminded me that
release cannot be called reliably from a JavaThread that is blocked.
The release must be done while the JavaThread is in the VM to safely
deal with Oop Storage.

> Is the safepoint check for each item in the list because the OopStorage.release() might take a while?

What safepoint check for each item? In this function, we're dealing with
just one ObjectMonitor and not a list. Is this comment in the wrong place?

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

PR: https://git.openjdk.org/jdk/pull/11296