Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8280168: Add Objects.toIdentityString #7139

Closed
wants to merge 9 commits into from

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Jan 18, 2022

While it is strongly recommend to not use the default toString for a class, at times it is the least-bad alternative. When that alternative needs to be used, it would be helpful to have the implementation already available, such as in Objects.toDefaultString(). This method is analagous to System.identityHashCode.

Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8280184


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7139/head:pull/7139
$ git checkout pull/7139

Update a local copy of the PR:
$ git checkout pull/7139
$ git pull https://git.openjdk.java.net/jdk pull/7139/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7139

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7139.diff

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jan 18, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 18, 2022

👋 Welcome back darcy! 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 openjdk bot added the rfr Pull request is ready for review label Jan 18, 2022
@openjdk
Copy link

openjdk bot commented Jan 18, 2022

@jddarcy 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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 18, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 18, 2022

/**
* {@return a string equivalent to the string returned by {@code
* Object.toString} if that method is not overridden}
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original Object.toString() invokes the hashCode() method.
Perhaps the spec should indicate that it calls IdentityhashCode() instead of hashCode().

Though not incorrect, in Valhalla, Not all objects have identity and will need to different implementation.

Are there any use cases in the JDK that will use this method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the return statement to say if hashCode isn't overridden either. The implSpec tag does explicitly mention identityHashCode already.

There are a few instances of this sort of code in the JDK src directory already; I may include a refactoring of at least one of them in a future revision of the PR. Thanks.

}
Object o = new Object(){};
errors += (Objects.toDefaultString(o).equals(o.toString()))? 0 : 1;
return errors;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another potential test is on an instance of a class that overrides toString(), that would ensure that toDefaultString() doesn't invoke toString.

Copy link
Member

@mlchung mlchung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I reviewed the CSR but please update the CSR with the updated spec.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@openjdk openjdk bot removed the rfr Pull request is ready for review label Jan 20, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 20, 2022
@jddarcy
Copy link
Member Author

jddarcy commented Jan 20, 2022

Expanding the scope of the proposed change, perhaps outside of a good cost/benefit ratio, there are now two new methods:

toDefaultString(Object o) // uses o.hashCode
toIdentityString(Object o) // uses System.identityHashCode(o)

The revision refactors two occurrences of the logic of toDefaultString with calls to the new method.

The toIdentityString method has the sometimes helpful property that no overridden methods of the object are called in constructing the string.

I'll revise the CSR once the final set of methods and their spec for this fix is determined.

@@ -292,7 +292,7 @@ private static boolean isObjectMethod(Method m) {
private static Object callObjectMethod(Object self, Method m, Object[] args) {
assert(isObjectMethod(m)) : m;
return switch (m.getName()) {
case "toString" -> self.getClass().getName() + "@" + Integer.toHexString(self.hashCode());
case "toString" -> java.util.Objects.toDefaultString(self);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better if toString is changed to invoke toIdentityString(self), only because hashCode returns the identity hash code, it doesn't invoke hashCode().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; that looks like an inconsistency/bug. Updated in a subsequent push.

Copy link
Member

@mlchung mlchung Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes,it should call the one with identity hash code to avoid the confusion. The toString method of a Proxy instance should return the result of the default implementation of Object::toString method as specified.

@stuart-marks
Copy link
Member

I'm wondering if we want to have toDefaultString at all, and whether we should have just toIdentityString. The primary use case, it seems to me, is the ability to get a string representation for some object, without involving any code in that object itself. There are the collections with cycles that I mentioned previously (in comments on the CSR), for which there is no well-defined hashCode. There is also the case that when logging mutable objects, the hashCode can change over time. This makes it difficult to analyze a log file and track what happens to a particular object. (I know I've been confused by this phenomenon in the past.) Thus toDefaultString doesn't seem all that useful to me.

It may be that the current use cases in the JDK can be replaced with toDefaultString but not toIdentityString without changing their behavior. But maybe we should consider changing their behavior and use toIdentityString instead.

@jddarcy jddarcy changed the title JDK-8280168 Add Objects.toDefaultString JDK-8280168: Add Objects.toIdentityString Jan 24, 2022
@jddarcy
Copy link
Member Author

jddarcy commented Jan 24, 2022

I'm wondering if we want to have toDefaultString at all, and whether we should have just toIdentityString. The primary use case, it seems to me, is the ability to get a string representation for some object, without involving any code in that object itself. There are the collections with cycles that I mentioned previously (in comments on the CSR), for which there is no well-defined hashCode. There is also the case that when logging mutable objects, the hashCode can change over time. This makes it difficult to analyze a log file and track what happens to a particular object. (I know I've been confused by this phenomenon in the past.) Thus toDefaultString doesn't seem all that useful to me.

It may be that the current use cases in the JDK can be replaced with toDefaultString but not toIdentityString without changing their behavior. But maybe we should consider changing their behavior and use toIdentityString instead.

Updated the PR to just have toIdentityString and make corresponding changes to the CSR.

@forax
Copy link
Member

forax commented Jan 24, 2022

toIdentityString is a better name than toDefaultString.

It's fine for me but given that "identity" has a slightly different meaning in the context of Valhalla that in System.identityHashCode(), it may be good to ask Brian about that name.

@jddarcy
Copy link
Member Author

jddarcy commented Jan 24, 2022

toIdentityString is a better name than toDefaultString.

It's fine for me but given that "identity" has a slightly different meaning in the context of Valhalla that in System.identityHashCode(), it may be good to ask Brian about that name.

Modulo the method name, I have discussed with Brian adding a method with toIdentityString's semantics to Objects and he agreed the method was worth having.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated proposal and naming looks okay.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jddarcy
Copy link
Member Author

jddarcy commented Jan 25, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jan 25, 2022

@jddarcy This pull request has not yet been marked as ready for integration.

@jddarcy
Copy link
Member Author

jddarcy commented Jan 25, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jan 25, 2022

@jddarcy This pull request has not yet been marked as ready for integration.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Jan 25, 2022
@openjdk
Copy link

openjdk bot commented Jan 25, 2022

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

8280168: Add Objects.toIdentityString

Reviewed-by: alanb, mchung, rriggs, smarks

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

  • f4575e4: 8279946: (ch) java.nio.channels.FileChannel tryLock and write methods are missing @throws NonWritableChannelException
  • 674a97b: 8280396: G1: Full gc mark stack draining should prefer to make work available to other threads
  • fe77250: 8280414: Memory leak in DefaultProxySelector
  • 496baad: 8280030: [REDO] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
  • 4503d04: 8280375: G1: Tighten mem region limit in G1RebuildRemSetHeapRegionClosure
  • 36fbec7: 8280241: (aio) AsynchronousSocketChannel init fails in IPv6 only Windows env
  • 28796cb: 8163921: HttpURLConnection default Accept header is malformed according to HTTP/1.1 RFC
  • c43ce85: 8278302: [s390] Implement fast-path for ASCII-compatible CharsetEncoders
  • 1b14157: 8280274: Guard printing code of Compile::print_method in PRODUCT
  • 2155afe: 8280503: Use allStatic.hpp instead of allocation.hpp where possible
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/1c7769d35b3a2aa4afe3125239dbfa1da5cfdeee...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 openjdk bot added the ready Pull request is ready to be integrated label Jan 25, 2022
@jddarcy
Copy link
Member Author

jddarcy commented Jan 25, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Jan 25, 2022

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

  • f4575e4: 8279946: (ch) java.nio.channels.FileChannel tryLock and write methods are missing @throws NonWritableChannelException
  • 674a97b: 8280396: G1: Full gc mark stack draining should prefer to make work available to other threads
  • fe77250: 8280414: Memory leak in DefaultProxySelector
  • 496baad: 8280030: [REDO] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
  • 4503d04: 8280375: G1: Tighten mem region limit in G1RebuildRemSetHeapRegionClosure
  • 36fbec7: 8280241: (aio) AsynchronousSocketChannel init fails in IPv6 only Windows env
  • 28796cb: 8163921: HttpURLConnection default Accept header is malformed according to HTTP/1.1 RFC
  • c43ce85: 8278302: [s390] Implement fast-path for ASCII-compatible CharsetEncoders
  • 1b14157: 8280274: Guard printing code of Compile::print_method in PRODUCT
  • 2155afe: 8280503: Use allStatic.hpp instead of allocation.hpp where possible
  • ... and 15 more: https://git.openjdk.java.net/jdk/compare/1c7769d35b3a2aa4afe3125239dbfa1da5cfdeee...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 25, 2022
@openjdk openjdk bot closed this Jan 25, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2022
@openjdk
Copy link

openjdk bot commented Jan 25, 2022

@jddarcy Pushed as commit cbe8395.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
6 participants