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

[Coleen Phillimore]

On Tue, 29 Nov 2022 21:11:20 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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?

Yeah, I was talking about the safepoint check for each monitor in the code in deflate_Idle_monitors, but replied to this comment because moving the release out of the ObjectMonitor destructor made you add this call (and it was a reply to what David said about ObjectSynchronizer knowing what ObjectMonitor nonstatic data members and that it needed to know how to destroy them.

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

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