8378531: [lworld] Remove obj_at(int) from objArrayOopDesc and flatArrayOopDesc

[xmas92]

obj_at(int) on flatArrayOopDesc should never be called as it may fail with an out of memory error and crash. Similarly objArrayOopDesc should not provide a shared interface to this member function which is only valid to call on a refArrayOopDesc.

This RFE cleans up the last potentially invalid uses by using and asserting stricter types at the call sites.

Most cleanup are straight forward, however there are a few parts where some extra feedback is appreciated.

First the primitive caches (ac32b67). JDK-8369921/#1685 conditioned the use of the caches behind the !PreviewFeatures.isEnabled() such that they are not used when running with Valhalla. However it did not do this for Character, even though both the issue and the PR mentions this Class as well, and could not find any discussion in the PR.

Update: @jsikstro pointed me to JDK-8372619, I am curious to where and how we access these caches (except for valueOf), as this change which removes the caches in Valhalla seems to run through our testing.

Second the caches were still created even if they were not used. In this PR I chose to not create these caches at all if we are in preview mode. Also I am not sure if it is the case that we always trust the internal final values and do fold them, but I changed it to use @stable checks vs a sentinel, which I know are trusted as final. However maybe this is unnecessary, and some core library / compiler person has some input.

Second is all the JVMCI changes (fe98ae0). This patch changes it so that the internal JVMCI Classes that that hotspot knows about are required to be RefArrays (this was already the assumption, but this hardens this by changing their types). And for external arrays they either throw an exception if it is an unexpected type (the case for executeHotSpotNmethod) or use obj_at(int, TRAP) and returns with an out of memory error.

Testing:

• Running tier 1-7 with `--enable-preview'

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8378531: [lworld] Remove obj_at(int) from objArrayOopDesc and flatArrayOopDesc (Enhancement - P4)

Reviewers

• Stefan Karlsson (@stefank - Committer)
• Frederic Parain (@fparain - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2157/head:pull/2157
$ git checkout pull/2157

Update a local copy of the PR:
$ git checkout pull/2157
$ git pull https://git.openjdk.org/valhalla.git pull/2157/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2157

View PR using the GUI difftool:
$ git pr show -t 2157

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2157.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back aboldtch! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@xmas92 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8378531: [lworld] Remove obj_at(int) from objArrayOopDesc and flatArrayOopDesc

Reviewed-by: stefank, fparain

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the lworld branch:

• 4e71493: 8378771: [lworld] JFR Cooperative Sampling support for needs_stack_repair methods and frames

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 09: Full (1bc75d0b)
• 08: Full - Incremental (f33c12f1)
• 07: Full - Incremental (174921f8)
• 06: Full - Incremental (a3926abe)
• 05: Full (cedcc96a)
• 04: Full - Incremental (c70c5359)
• 03: Full - Incremental (209f6254)
• 02: Full (e417b2a5)
• 01: Full (2dd346f3)
• 00: Full (138cca9e)

[marc-chevalier]

I went through but that's too much runtime-related for me to say much. Indeed, most of the changes seems very direct.

And thanks for doing that, it will be less confusing. I've used flatArrayOopDesc::obj_at(int) before the fatal was added.

I just wonder whether we should add a warning on flatArrayOopDesc::obj_at(int index, TRAPS) quickly stating why the obj_at(int) version doesn't exist so nobody is tempted to reintroduce it. It's tempting, the ..., TRAPS) version often have a version without. Until Frederic explained me what happens there, I think I could have (re-)added it.

[walulyai]

I just wonder whether we should add a warning on flatArrayOopDesc::obj_at(int index, TRAPS) quickly stating why the obj_at(int) version doesn't exist so nobody is tempted to reintroduce it. It's tempting, the ..., TRAPS) version often have a version without. Until Frederic explained me what happens there, I think I could have (re-)added it.

can we add inline oop obj_at(int index) const = delete; with the appropriate comment?

[xmas92]

I just wonder whether we should add a warning on flatArrayOopDesc::obj_at(int index, TRAPS) quickly stating why the obj_at(int) version doesn't exist so nobody is tempted to reintroduce it. It's tempting, the ..., TRAPS) version often have a version without. Until Frederic explained me what happens there, I think I could have (re-)added it.

can we add inline oop obj_at(int index) const = delete; with the appropriate comment?

Added a deleted function on objArrayOopDesc. Also added a deprecation message in hopes that some current or future compiler understands this as a message for the delete, or when we eventually get to C++26 we can add it directly to the delete statement. Also tried just the depreciation, but it still marks the method as valid from most IDEs / LSPs, even though we compile with deprecation warnings and warnings as errors.

I also added a comment and deleted this function in flatArrayOopDesc, I am not sure that warrants a deprecation message, as there is no usable alternative suggestion we can give.

And finally I added inline bool obj_at_is_null(int index) const; to flatArrayOopDesc and objArrayOopDesc so we can have a none trapping null check.

[marc-chevalier]

Very nice, I would have been happy with just delete or comment, but why not both!

Isn't flatArrayOopDesc::obj_at_is_null semantically very similar to flatArrayOopDesc::null_marker_of_obj_at? Except that "object == null <=> null marker == 0" (it can be confusing to see that a true null marker means non-null, but I think about it as the null marker has the same nullness as the object).

[xmas92]

Isn't flatArrayOopDesc::obj_at_is_null semantically very similar to flatArrayOopDesc::null_marker_of_obj_at? Except that "object == null <=> null marker == 0" (it can be confusing to see that a true null marker means non-null, but I think about it as the null marker has the same nullness as the object).

We also have that flatArrayOopDesc::obj_at_is_null returns false for a null restricted array, while flatArrayOopDesc::null_marker_of_obj_at is invalid to call on a null restricted array (might crash, assert or return an incorrect answer). (We might want to assert this precondition directly in null_marker_of_obj_at, it is also unclear why it has a TRAP overload, but maybe that is vestigial.)

I cannot see that we currently use null_marker_of_obj_at. It looks like you introduced this. I assumed it was for the compiler, returning a jboolean is not very useful in the C++ code.

I was thinking of implementing null_marker_of_obj_at in terms of obj_at_is_null. I also plan to go over and look form more of the explicit code that does value payload accesses and evaluate if using the ValuePayload abstraction is better.

[marc-chevalier]

I think where it was used was removed later. I think it was used for constant folding. In case we had a constant object, we could need to query its null marker and replace a load by a constant with with jboolean, so I think it was the right type. But indeed, it was only for nullable things, but it made sense: we shouldn't have a load trying to get the null marker of a null-restricted thing in the first place.

[stefank]

I looked through the C++ code (except JVMCI) and I think this looks good. I have one question below:

[fparain]

Looks much safer and cleaner with your changes.
However, this patch touches the code of the caches of the wrapper classes. Is this being done in coordination with with JDK-8379148 and JDK-8370724 ?

[xmas92]

However, this patch touches the code of the caches of the wrapper classes. Is this being done in coordination with with JDK-8379148 and JDK-8370724 ?

After the latest merge the all caches area disabled from the Java code (the interim re-enabling of the Character cache for JDK-8370724 has been reverted). So this patch should not change much for that issue.

As for JDK-8379148, I had not observed its existence, but I would say that this patch resolves that issue. @Arraying as the assignee of that issue, do you agree, or are there more primitive cache accesses from the VM you are looking at that this patch does not address?

[Arraying]

However, this patch touches the code of the caches of the wrapper classes. Is this being done in coordination with with JDK-8379148 and JDK-8370724 ?

After the latest merge the all caches area disabled from the Java code (the interim re-enabling of the Character cache for JDK-8370724 has been reverted). So this patch should not change much for that issue.

As for JDK-8379148, I had not observed its existence, but I would say that this patch resolves that issue. @Arraying as the assignee of that issue, do you agree, or are there more primitive cache accesses from the VM you are looking at that this patch does not address?

So far I've only found VM usage of the primitive cache accesses during deoptimization which (after taking a quick look) seem to be addressed in your patch. I can make 8379148 blocked by this PR, take a step back for a bit until this change is in, and then if there's anything else integrate a followup and otherwise (and likely) close 8379148. Would that be okay with you?

[xmas92]

Would that be okay with you?

Sounds good!

[xmas92]

I reverted the JDK primitive cache changes. (So the caches are still constructed, just never used with enable preview). There were some unforeseen interactions with AOT / CDS, causing a handful of serviceability tests to fail unless ran with -Xshare:off. I will create a separate RFE for investigating removing the primitive caches from the enable preview archive.

[xmas92]

Thanks for the reviews.
Ran through the testing tier 1-7 with --enable-preview. There were a few issues but all seem present in lworld as well.
Will check if some of them are not tracked in JBS yet.

/integrate

[openjdk]

Going to push as commit f81526c.
Since your change was applied there has been 1 commit pushed to the lworld branch:

• 4e71493: 8378771: [lworld] JFR Cooperative Sampling support for needs_stack_repair methods and frames

Your commit was automatically rebased without conflicts.

[openjdk]

@xmas92 Pushed as commit f81526c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.