8346609: Improve MemorySegment.toString by minborg · Pull Request #22826 · openjdk/jdk

minborg · 2024-12-19T09:48:23Z

This PR proposes to improve the current MemorySegment.toString() method from:

MemorySegment{ address: 0x60000264c540, byteSize: 8 }

to

MemorySegment{ native, address: 0x60000264c540, byteSize: 8}

Tests passes tier1-tier3

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

Issue

JDK-8346609: Improve MemorySegment.toString (Enhancement - P4)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22826

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-19T09:49:28Z

👋 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 · 2024-12-19T09:50:33Z

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

8346609: Improve MemorySegment.toString

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

5e6cda4: 8347000: Bug in com/sun/net/httpserver/bugs/B6361557.java test
3f7052e: 8346868: RISC-V: compiler/sharedstubs tests fail after JDK-8332689
f119663: 8346573: Can't use custom default file system provider with custom system class loader
379ac34: 8346838: RISC-V: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java crash with debug VMs
0285020: 8345676: [ubsan] ProcessImpl_md.c:561:40: runtime error: applying zero offset to null pointer on macOS aarch64
0a81676: 8346881: [ubsan] logSelection.cpp:154:24 / logSelectionList.cpp:72:94 : runtime error: applying non-zero offset 1 to null pointer

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 master branch, type /integrate in a new comment.

openjdk · 2024-12-19T09:51:35Z

@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 · 2024-12-19T09:54:44Z

Webrevs

JornVernee

I'm not sure if all this information is useful to have in the toString output. I'll note that all of this information is publicly accessible through the MermorySegment interface aswell, so could be printed with additional print statements. Except for confined-ness, which is exposed through isAccessibleBy instead, since confined-ness is not a property of memory segments, but of arenas (and I don't think we should expose it directly like this).

Subjectively, I think the things that are reasonable to expose in the toString output are the things that are already exposed today: the heap base, address/offset, and size. I would find it reasonable to add isAlive=true/false to that, but it's not a property of the MS itself, but of the scope that it is attached to, so I'm not sure. But, read-only, and mapped are things I don't think are relevant often enough to include.

If you want a complete overview of the properties of the object, I think what you really want is a debugger :)

mcimadamore · 2024-12-20T09:46:53Z

I would find it reasonable to add isAlive=true/false to that, but it's not a property of the MS itself, but of the scope that it is attached to, so I'm not sure.

And even for liveness, the fact that this info could be updated concurrently makes it a bit hard to capture in the toString output - e.g. "was alive when printed" :-)

mcimadamore · 2025-01-07T11:44:50Z

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

        return "MemorySegment{ " +
-                heapBase().map(hb -> "heapBase: " + hb + ", ").orElse("") +
-                "address: " + Utils.toHexString(address()) +
+                heapBase().map(hb -> "heapBase: " + hb).orElse(isMapped() ? "mapped" : "native") +


This is not correct. Heap base can be empty for read-only segments.
Have we considered printing one of:

HeapMemorySegment

NativeMemorySegment

MappedMemorySegment

E.g. add the heap/native/mapped as part of the "class" qualifier, before the { ? That seems more readable (of course the drawback is that it might suggest that these classes exist somewhere, which they don't.

A possible compromise would be:

MemorySegment{ kind: heap/native/mapped, [heapBase: xyz ], address: xyz, size: xyz}

This is consistent with the terminology found in the javadoc:

There are two kinds of memory segments: ...

mcimadamore · 2025-01-07T13:00:08Z

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

-        return "MemorySegment{ " +
-                heapBase().map(hb -> "heapBase: " + hb).orElse(isMapped() ? "mapped" : "native") +
+        final String kind;
+        if (this instanceof HeapMemorySegmentImpl) {


could also be a "virtual" method

minborg · 2025-01-07T15:17:27Z

/integrate

openjdk · 2025-01-07T15:18:05Z

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

4d8fb80: 8347038: [JMH] jdk.incubator.vector.SpiltReplicate fails NoClassDefFoundError
cf3e48e: 8346965: Multiple compiler/ciReplay test fails with -XX:+SegmentedCodeCache
e5f0c19: 8345041: IGV: Free Placement Mode in IGV Layout
8b22517: 8211851: (ch) java/nio/channels/AsynchronousSocketChannel/StressLoopback.java times out (aix)
5e6cda4: 8347000: Bug in com/sun/net/httpserver/bugs/B6361557.java test
3f7052e: 8346868: RISC-V: compiler/sharedstubs tests fail after JDK-8332689
f119663: 8346573: Can't use custom default file system provider with custom system class loader
379ac34: 8346838: RISC-V: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java crash with debug VMs
0285020: 8345676: [ubsan] ProcessImpl_md.c:561:40: runtime error: applying zero offset to null pointer on macOS aarch64
0a81676: 8346881: [ubsan] logSelection.cpp:154:24 / logSelectionList.cpp:72:94 : runtime error: applying non-zero offset 1 to null pointer

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-07T15:18:13Z

@minborg Pushed as commit c8a9dd3.

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

minborg added 2 commits December 17, 2024 13:49

Improve MemorySegment::toString

4eea51d

Implement an alternate MS::toString

73efff4

openjdk bot changed the title ~~JDK-8346609: Improve MemorySegment.toString~~ 8346609: Improve MemorySegment.toString Dec 19, 2024

openjdk bot added the rfr Pull request is ready for review label Dec 19, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Dec 19, 2024

JornVernee reviewed Dec 19, 2024

View reviewed changes

minborg added 2 commits January 7, 2025 12:10

Merge branch 'master' into ms-tostring-improvement2

06b5450

Reduce elements in toString

d960678

mcimadamore reviewed Jan 7, 2025

View reviewed changes

minborg added 4 commits January 7, 2025 13:13

Update segment type logic

08ed7c7

Add 'kind'

6253e0e

Fix extraoneous comma

dea2fc3

Rename variable

ad0fd94

mcimadamore reviewed Jan 7, 2025

View reviewed changes

mcimadamore approved these changes Jan 7, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 7, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 7, 2025

openjdk bot closed this Jan 7, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8346609: Improve MemorySegment.toString#22826

8346609: Improve MemorySegment.toString#22826
minborg wants to merge 8 commits intoopenjdk:masterfrom
minborg:ms-tostring-improvement2

minborg commented Dec 19, 2024 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Dec 19, 2024

Uh oh!

openjdk bot commented Dec 19, 2024 •

edited

Loading

Uh oh!

openjdk bot commented Dec 19, 2024

Uh oh!

mlbridge bot commented Dec 19, 2024 •

edited

Loading

Uh oh!

JornVernee left a comment

Uh oh!

mcimadamore commented Dec 20, 2024

Uh oh!

mcimadamore Jan 7, 2025 •

edited

Loading

Uh oh!

mcimadamore Jan 7, 2025

Uh oh!

minborg commented Jan 7, 2025

Uh oh!

openjdk bot commented Jan 7, 2025

Uh oh!

openjdk bot commented Jan 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

minborg commented Dec 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 19, 2024

Uh oh!

openjdk bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 19, 2024

Uh oh!

mlbridge bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

JornVernee left a comment

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Dec 20, 2024

Uh oh!

mcimadamore Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

minborg commented Jan 7, 2025

Uh oh!

openjdk bot commented Jan 7, 2025

Uh oh!

openjdk bot commented Jan 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

minborg commented Dec 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 19, 2024 •

edited

Loading

mlbridge bot commented Dec 19, 2024 •

edited

Loading

mcimadamore Jan 7, 2025 •

edited

Loading