RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v26]

[Scott Gibbons]

On Fri, 27 Sep 2024 08:24:50 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> @rkennke I reviewed [rkennke@ 097c2af](https://github.com/rkennke/jdk/commit/097c2afa04397773e514552dfb942aa889bfa2c1) and the code looks good to me.  I would prefer this approach instead of not generating the IndexOf intrinsic.
>> 
>> Should the controlling `if` be conditioned on `UseCompactObjectHeaders` instead of `arrayOopDesc::base_offset_in_bytes`?  I can see benefits to either - which provides more clarity?  I like the assert as it makes the intention clear (thanks!).
>
> I like to have the functional connection: if - for whatever reason - the array base offset is smaller than 16, we need to deal with that. The reason for this happens to be `UseCompactObjectHeaders`, but that may not be clear to the reader of the code. I could add an `assert(UseCompactObjectHeaders` in that branch to make that connection clear. Also consider that `UseCompactObjectHeaders` is intended to go away at some point.
> 
> I wonder if having 2 or 3 branches ahead of the main-loop (which probably doesn't do much, because haystack is <=32 bytes) is a useful approach, or if there may be a better way to get the bytes on the stack? I don't know enough about the implementation to make that judgement.

I believe the code in the patch is good enough as-is, especially if `UseCompactObjectHeaders` is slated to go away.  The existing `if` will prevent the < 16 byte header code from being emitted, which is the desired behavior - i.e., if the header size is >= 16, there will be no code emitted to the intrinsic for that block.  So there will not be an additional branch for the code when it is executed.

I'm good with a comment tying `UseCompactObjectHeaders` to the condition.  The comment can be removed when the flag is removed.  "Ship it" :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1778739517