RFR: 8338658: New Object to ObjectMonitor mapping: s390x implementation [v2]

[Axel Boldt-Christmas]

On Sun, 8 Sep 2024 11:12:24 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> Amit Kumar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review comments
>
> src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6023:
> 
>> 6021: 
>> 6022:   if (UseObjectMonitorTable) {
>> 6023:     // Clear cache in case fast locking succeeds.
> 
> @xmas92: This comment sounds like it should only be done if fast locking succeeds. Why are we doing it regardless of that?

The invariants surrounding the cache went back and forth a bit. 

The important part is that the cache slot in the `BasicLock` is valid after the enter is complete. There is now a RAII object in the runtime code which makes sure this is the case for all paths. So it should be the case that the C2 code only needs to handle the case where it is successful. (Clearing when fast lock succeeds and storing the monitor when inflated locking succeeds.)

I think it went in in this state as a combination of it being something that once was required (because the runtime was less precise with how it handles the `BasicLock` cache) and that I wanted to take the C2 changes in as they were because most of the performance testing had been performed in that state. Maybe there was some technical issue somewhere with regards to register availability, cannot recall. Regardless if it can be only done in the none slow path case and it is more performant it should be fine to do so.

I do not think I saw a measurable difference on x86_64 from having the two stores in the successful inflated case. And in all other cases the only time you can elide the store is when the slow path is taken, so it probably does not save much if anything.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20740#discussion_r1749678191