RFR: 8324881: ObjectSynchronizer::inflate(Thread* current...) is invoked for non-current thread [v2]

[David Holmes]

On Wed, 31 Jan 2024 01:37:42 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add regression test
>
> Many thanks for picking this up @xmas92 . 
> 
> I'm very frustrated that this code got broken the way it has been. Given there are only two options here: actual current thread or else a JVMTI suspended thread we are processing on-behalf of, I'm very tempted to introduce a new API for the latter case to use e.g.: `inflate_for` and `ObjectMonitor::set_owner_to`. When dealing with the suspended thread only very limited situations are actually possible so we don't need all the possible cases to be checked in `inflate` or `enter` - it can just be asserted what state the object/monitor must be in. We can then revert inflate/enter to always and only, act on the current thread. I think the resulting code would be much simpler to understand overall.

> @dholmes-ora I've introduced the code and I am very offended about the way you express your feelings here and elsewhere about it. 

@reinrich  I am sorry if you are offended, that was not my intent, but the code was broken. Two methods clearly had a pre-requisite that they expected to be passed the current thread as "current" and we use that parameter name for clarity so anyone reading the code can safely assume that it is in fact the current thread. We did a lot of code cleanup in this area a few years ago to make it so. At the time I resisted requests to add assertions everywhere to verify that as I thought it unnecessary as the intent was clear, but if they had been present they would have caught this - so that omission is on me. It is perfectly fine that the code can be, and now is, passed a non-current thread, but that should have been made clear through a change to the parameter name - as is now happening - and corrected use of the ResourceMarks. Unfortunately this was also missed by the reviewers at the time, including my partial review (deopt is not my area so I did not look at the code in detail).

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

PR Comment: https://git.openjdk.org/jdk/pull/17626#issuecomment-1920579773