8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.

[minborg]

This PR proposes to explicitly state that returned segments form the asSlice and reinterpret method share memory regions with this memory segment.

Note: The term "subset" means a true subset or the same set.

---

Progress

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

Warning

 ⚠️ Found trailing period in issue title for 8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.

Issue

• JDK-8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory. (Enhancement - P4)

Reviewers

• Jorn Vernee (@JornVernee - Reviewer)
• Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19633

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19633.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

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

8333886: Explicitly specify that asSlice and reinterpret return a memory segment backed by the same region of memory.

Reviewed-by: jvernee, mcimadamore

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 24 new commits pushed to the master branch:

• aaaa86b: 8333360: PrintNullString.java doesn't use float arguments
• ef101f1: 8332920: C2: Partial Peeling is wrongly applied for CmpU with negative limit
• 2843745: 8333972: Parallel: Remove unused methods in PSOldGen
• 93f3918: 8333954: Parallel: Remove unused arguments of type ParCompactionManager*
• 788b876: 8333917: G1: Refactor G1CollectedHeap::register_old_region_with_region_attr
• 0e4d4a0: 8320725: AArch64: C2: Add "requires_strict_order" flag for floating-point add and mul reduction
• badf1cb: 8331675: gtest CollectorPolicy.young_min_ergo_vm fails after 8272364
• 4d6064a: 8333649: Allow different NativeCall encodings
• fe9c63c: 8333931: Problemlist serviceability/jvmti/vthread/CarrierThreadEventNotification
• 41c88bc: 8333756: java/lang/instrument/NativeMethodPrefixApp.java failed due to missing intrinsic
• ... and 14 more: https://git.openjdk.org/jdk/compare/83b34410e326c47f357a37c3a337b7dedb8cbbda...master

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 master branch, type /integrate in a new comment.

[openjdk]

@minborg The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (13f33e65)
• 00: Full (6cde00c8)

[JornVernee]

The term 'subset' doesn't feel right to me here, since we're only talking about a single memory region (not a set of memory regions). I suggest 'sub-region' instead.

[JornVernee]

The term 'subset' doesn't feel right to me here, since we're only talking about a single memory region (not a set of memory regions). I suggest 'sub-region' instead.

Actually, nvm, that doesn't work for reinterpret since the returned segment can also cover a larger region of memory.

[tkslaw]

The term 'subset' doesn't feel right to me here, since we're only talking about a single memory region (not a set of memory regions). I suggest 'sub-region' instead.

Actually, nvm, that doesn't work for reinterpret since the returned segment can also cover a larger region of memory.

In my opinion, any 'sub' terminology is misleading since, as you pointed out, one can (and often will) reinterpret to a larger size. Wouldn't something like, "The returned memory segment is backed by the same region of memory that backs this memory segment", be sufficiently accurate? Could even append, "No memory will be allocated or deallocated", to really hammer the point home.

Using 'subset' for asSlice makes sense. Though possibly unnecessary.

[minborg]

In my opinion, any 'sub' terminology is misleading since, as you pointed out, one can (and often will) reinterpret to a larger size. Wouldn't something like, "The returned memory segment is backed by the same region of memory that backs this memory segment", be sufficiently accurate? Could even append, "No memory will be allocated or deallocated", to really hammer the point home.

Using 'subset' for asSlice makes sense. Though possibly unnecessary.

If a segment is reinterpreted to be larger than this segment, then the extra memory is not a part of this segment's backing part. So, in this case the total memory is a subset (but not a proper subset but rather the same) of this backing memory plus some additional memory.

If a segment is reinterpreted to be smaller than this segment, then the reduced memory is a proper subset.

So, strictly the proposed text is correct. The proposition "The returned memory segment is backed by the same region of memory that backs this memory segment" appears to be incorrect if a smaller chunk is carved out?

Maybe something more neutral like "The returned memory segment shares a region of backing memory with this segment"?

Adding text similar to "No memory will be allocated or deallocated" sounds like a good idea.

[JornVernee]

If a segment is reinterpreted to be larger than this segment, then the extra memory is not a part of this segment's backing part.

Another way to think about this is: a segment's backing region can be larger or smaller than the bounds specified by the segment itself. Though, in most cases they are the same. For example, If we allocate 10 bytes of memory in native code, and return it to the Java side, I'd argue that the backing region of the memory segment we get back is still 10 bytes in size, even though the byteSize() of the memory segment is 0. reinterpret can be used to adjust the size of the segment, but it doesn't change the size of the underlying memory region, which remains 10 bytes.

[JornVernee]

Latest version looks good to me.

[minborg]

/integrate

[openjdk]

Going to push as commit c80e2eb.
Since your change was applied there have been 34 commits pushed to the master branch:

• a0318bc: 8334077: Fix problem list entries for compiler tests
• a7e4ab9: 8333730: ubsan: FieldIndices/libFieldIndicesTest.cpp:276:11: runtime error: null pointer passed as argument 2, which is declared to never be null
• abbf45b: 8332699: ubsan: jfrEventSetting.inline.hpp:31:43: runtime error: index 163 out of bounds for type 'jfrNativeEventSetting [162]'
• bd046d9: 8222884: ConcurrentClassDescLookup.java times out intermittently
• 1c80ddb: 8333940: Ensure javax/swing/TestUngrab.java run on all platforms
• a7205cc: 8333926: Shenandoah: Lower default immediate garbage threshold
• 56e8e60: 8330534: Update nsk/jdwp tests to use driver instead of othervm
• bbd3b1d: 8334036: Update JCov for class file version 68
• 7ed8a5c: 8333841: Add more logging into setfldw001 tests
• b77bd5f: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259
• ... and 24 more: https://git.openjdk.org/jdk/compare/83b34410e326c47f357a37c3a337b7dedb8cbbda...master

Your commit was automatically rebased without conflicts.

[openjdk]

@minborg Pushed as commit c80e2eb.

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