8291473: Unify MemorySegment and MemoryAddress

[mcimadamore]

This patch implements the changes described in [1].

The main things to notice are the removal of MemoryAddress and Addressable. Everything else falls from that.

There are some changes to the MemorySegment API as well, as some methods such as segmentOffset and asOverlappingSlice have been removed, and instead MemorySegment::address now works on heap segments too (and, clients can obtain the array of an heap segment using its array method). So, these methods can, if needed, be implemented outside the core API.

MemorySegment::equals now compares the segments address, which is the most useful comparison when working with native interop (again, client can implement deeper comparison using mismatch).

Finally, restricted factories accepting raw addresses (in MemorySegment and VaList) have been tweaked to accept a long value instead.

VaList also needed some adjustments, since it builds on top of segments. The API is similar to before - but instead of having an address accessor, it has a zero-length segment accessor.

ValueLayout.OfAddress has a new method to create unsafe address layouts which return segment with maximal size (Long.MAX_VALUE). I've used these layouts in a lot of places in the implementation internal, which simplifies things quite a bit (e.g. removing the need to create a new segment from an address).

There are several test and microbenchmark updates, but relatively minor, and all caused by removal of MemoryAddress/Addressable. Changes here should be relatively simple to follow.

While the javadoc is ok, I didn't put too much effort in trying to provide a complete and cohesive story on how zero-length segments are used to model raw addresses. I'm working on some more extensive doc changes on the side, but I wanted to split the javadoc changes from the impl changes, so as to simplify the review work. I hope that makes sense.

[1] - https://mail.openjdk.org/pipermail/panama-dev/2022-July/017181.html

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8291473: Unify MemorySegment and MemoryAddress

Reviewers

• Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to 142dd031

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign pull/694/head:pull/694
$ git checkout pull/694

Update a local copy of the PR:
$ git checkout pull/694
$ git pull https://git.openjdk.org/panama-foreign pull/694/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 694

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/694.diff

[bridgekeeper]

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

[mlbridge]

Webrevs

• 11: Full - Incremental (6de1fa76)
• 10: Full - Incremental (142dd031)
• 09: Full - Incremental (98ece3e3)
• 08: Full - Incremental (390c52f3)
• 07: Full - Incremental (5e6794d9)
• 06: Full - Incremental (b8e59b10)
• 05: Full - Incremental (3543e669)
• 04: Full - Incremental (18e7f605)
• 03: Full - Incremental (2312cb30)
• 02: Full - Incremental (22f8e4c7)
• 01: Full - Incremental (118cf04b)
• 00: Full (31fa30b5)

[JornVernee]

but I wanted to split the javadoc changes from the impl changes, so as to simplify the review work. I hope that makes sense.

This is very appreciated.

[JornVernee]

There seems to be some comments (and 1 string) that mention MemoryAddress. Please update those as well.

[rsmogura]

I wonder if we should use here ofNullable

[rsmogura]

I would like to recall this comment. Even if it would look better from language perspective, having not specialized Optional can make Unsafe operations slower, as the VM can make issues with detecting if it's on heap or off-heap access.

[rsmogura]

I wonder if array word is not too strict, I think what if we would use value object as a base for struct (sometime in future).

[mcimadamore]

I wonder if array word is not too strict, I think what if we would use value object as a base for struct (sometime in future).

I think you are correct, the word might indeed be too strict. "Heap container" is what we're after, but I'm not sure if having a method named "heapContainer" would fly?

[rsmogura]

I'm not good at finding names. I got used to base. Maybe heapBase or heapObject. I'll think about it and come back if I invite something worth of mentioning.

[rsmogura]

I think heapContainer is good, I just thought as well about something like holder, or value holder if we would like to have something shorter - I'm really not an expert at naming.

[mcimadamore]

what about heapBase ?

[mcimadamore]

I'll keep the name (array) for now, we can always revise that later. Thanks.

[mcimadamore]

I've finished addressing the review comments. Now Binding.ToSegment has been removed, and Binding.BoxAddress does all the work. I think it looks clean.

[JornVernee]

I think this is mostly good now.

The only thing is that there's still some occurrence of MemoryAddress (when I search for it in src/java.base/classes), mostly in comments. Some of these are already stale, so not that important to fix, but there's also a reference to MemoryAddress in the class javadoc of SymbolLookup which I think should be updated.

[mcimadamore]

I think this is mostly good now.

The only thing is that there's still some occurrence of MemoryAddress (when I search for it in src/java.base/classes), mostly in comments. Some of these are already stale, so not that important to fix, but there's also a reference to MemoryAddress in the class javadoc of SymbolLookup which I think should be updated.

I'll take a pass

[mcimadamore]

I think this is mostly good now.
The only thing is that there's still some occurrence of MemoryAddress (when I search for it in src/java.base/classes), mostly in comments. Some of these are already stale, so not that important to fix, but there's also a reference to MemoryAddress in the class javadoc of SymbolLookup which I think should be updated.

I'll take a pass

I've fixed most of the spurious references to the now removed classes. I've left the main Binding javadoc alone, because it's full of references to stuff that's no longer valid, and should probably be rewritten in a separate pass.

[JornVernee]

Thanks. Looks good

[openjdk]

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

8291473: Unify MemorySegment and MemoryAddress

Reviewed-by: jvernee

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 65 new commits pushed to the foreign-memaccess+abi branch:

• 4696051: Automatic merge of master into foreign-memaccess+abi
• a708748: Automatic merge of jdk:master into master
• 18cd16d: 8291003: ARM32: constant_table.size assertion
• cfe9026: 8289511: Improve test coverage for XPath Axes: child
• eeac3da: 8289755: Remove --enable-reproducible-build from jib profile
• 791fc57: 6383195: javax.crypto.spec.PBEKeySpec is not thread safe
• e052d7f: 8288477: nmethod header size reduction
• 54a2c5a: 8290059: Do not use std::thread in panama tests
• 5214a17: 8291479: ProblemList compiler/rangechecks/TestRangeCheckHoistingScaledIV.java on ppc64le
• 471a427: 8287794: Reverse*VNode::Identity problem
• ... and 55 more: https://git.openjdk.org/panama-foreign/compare/bb2ef3e25abd6d4fb1286f58ea013eb5afe12b25...foreign-memaccess+abi

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 foreign-memaccess+abi branch, type /integrate in a new comment.

[mcimadamore]

/integrate

[openjdk]

Going to push as commit 8b1af9a.
Since your change was applied there have been 65 commits pushed to the foreign-memaccess+abi branch:

• 4696051: Automatic merge of master into foreign-memaccess+abi
• a708748: Automatic merge of jdk:master into master
• 18cd16d: 8291003: ARM32: constant_table.size assertion
• cfe9026: 8289511: Improve test coverage for XPath Axes: child
• eeac3da: 8289755: Remove --enable-reproducible-build from jib profile
• 791fc57: 6383195: javax.crypto.spec.PBEKeySpec is not thread safe
• e052d7f: 8288477: nmethod header size reduction
• 54a2c5a: 8290059: Do not use std::thread in panama tests
• 5214a17: 8291479: ProblemList compiler/rangechecks/TestRangeCheckHoistingScaledIV.java on ppc64le
• 471a427: 8287794: Reverse*VNode::Identity problem
• ... and 55 more: https://git.openjdk.org/panama-foreign/compare/bb2ef3e25abd6d4fb1286f58ea013eb5afe12b25...foreign-memaccess+abi

Your commit was automatically rebased without conflicts.

[openjdk]

@mcimadamore Pushed as commit 8b1af9a.

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