8366705: [lworld] Re-work of arrays meta-data

[fparain]

Since the removal of Q-types and the notion that nullability was not part of the Java type, there was an awkward situation because nullable arrays of value types and null free arrays of value types had each a different Java mirror when they were in fact supposed to have the same Java type.
In order to accommodate to the new situation, that arrays can have properties (nullability, flatness, atomicity, etc.) that are not part of their Java type, the 1-1 relationship between the *ArrayKlass and the Java mirror must be broken.
The proposed solution is to dedicate one instance of ObjArrayKlass to represent the Java type of the array in the JVM, and have this instance being the counterpart of the Java mirror of the array, and have several instances of RefArrayKlass and FlatArrayKlass that represent the refinements of the Java array type. Each RefArrayKlass/FlatArrayKlass encodes the characteristic of a Java array for a given element type and a set of properties.

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8366705: [lworld] Re-work of arrays meta-data (Bug - P3)

Reviewers

• Coleen Phillimore (@coleenp - Committer) ⚠️ Review applies to ca75bab8
• Tobias Hartmann (@TobiHartmann - Committer) ⚠️ Review applies to 527a17b6
• David Simms (@MrSimms - Committer) ⚠️ Review applies to 527a17b6

Contributors

• Tobias Hartmann <[email protected]>
• Coleen Phillimore <[email protected]>
• Chen Liang <[email protected]>
• Matias Saavedra Silva <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1452

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back fparain! 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]

@fparain 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:

8366705: [lworld] Re-work of arrays meta-data

Co-authored-by: Tobias Hartmann <[email protected]>
Co-authored-by: Coleen Phillimore <[email protected]>
Co-authored-by: Chen Liang <[email protected]>
Co-authored-by: Matias Saavedra Silva <[email protected]>
Reviewed-by: coleenp, thartmann, dsimms

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 10 new commits pushed to the lworld branch:

• 3620108: 8359370: [lworld] allow instance fields of identity classes to be readable in the prologue phase
• 4b93af7: 8366440: [lworld] Two tests broken with JDK-8364483
• 858be30: 8366879: [lworld] C2's lock elimination fails after JDK-8335256
• ... and 7 more: https://git.openjdk.org/valhalla/compare/22e9d5f58415d20d89a96a3bbcb6d7fffde9f8c1...lworld

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.

[coleenp]

Makes sense - found a typo. Are you going to check this in with UseNewCode2 or you could add a global flag for this because I think you're going to need a new one?

[openjdk]

@fparain this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout array_klasses
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

[bridgekeeper]

@fparain This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[fparain]

/keepalive

[openjdk]

@fparain The pull request is being re-evaluated and the inactivity timeout has been reset.

[fparain]

/contributor add @matias9927

[openjdk]

@fparain
Contributor Coleen Phillimore <[email protected]> successfully added.

[openjdk]

@fparain
Contributor Chen Liang <[email protected]> successfully added.

[openjdk]

@fparain
Contributor Matias Saavedra Silva <[email protected]> successfully added.

[mlbridge]

Webrevs

• 07: Full - Incremental (0e8cd32b)
• 06: Full - Incremental (ca75bab8)
• 05: Full - Incremental (b27def57)
• 04: Full - Incremental (c3327d60)
• 03: Full (527a17b6)
• 02: Full - Incremental (9b254f2e)
• 01: Full - Incremental (6087dbda)
• 00: Full (810a36fa)

[coleenp]

I reviewed the runtime/oops/interpreter code. My comments are questions and very minor suggestions, if you want to do them now. This is a very nice cleanup of the existing lworld repo array code.

[MrSimms]

Coleen beat me to most of the formatting issues and comment of deriving flatArray

Overall, this looking awesome.

One issue of concern: refArray and flatArray are distinct from any mainline code. But any cases of objArray changes in mainline, when merged may actually need changing to refArray. Especially if there is additional mainline code that does not overlap, i.e. there will no textual conflict and it will compile.

I took another pass at the PR looking at assertions when need refined types. I think you have it covered. That does mean more assertions during merge testing, but they should obvious.

I'm not even going to complain about how many textual conflicts this PR will produce in future mainline merging, those are up front and trivial to deal with.

If we were interested in catching merge problems earlier, we code rename objArray...but that feels extreme.

[coleenp]

This looks great. Nice rework and cleanups.

[fparain]

Many thanks to all contributors and all reviewers for helping on this huge work.

[fparain]

/integrate

[openjdk]

Going to push as commit 94cca4b.
Since your change was applied there have been 11 commits pushed to the lworld branch:

• b53b049: 8365978: [lworld] C2: assert(vk->maybe_flat_in_array()) when using compareAndSetFlatValue with -XX:-UseArrayFlattening
• 3620108: 8359370: [lworld] allow instance fields of identity classes to be readable in the prologue phase
• 4b93af7: 8366440: [lworld] Two tests broken with JDK-8364483
• ... and 8 more: https://git.openjdk.org/valhalla/compare/22e9d5f58415d20d89a96a3bbcb6d7fffde9f8c1...lworld

Your commit was automatically rebased without conflicts.

[openjdk]

@fparain Pushed as commit 94cca4b.

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