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

8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set #2178

Closed
wants to merge 10 commits into from

Conversation

navyxliu
Copy link
Member

@navyxliu navyxliu commented Jan 21, 2021

Add a flag _suppress_cr to outputStream. outstream objects won't emit any CR if it's set.
Correct TypeInstPtr::dump2 to make sure it only emits klass name once.
Remove the comment because Klass::oop_print_on() has emitted the address of oop.

Before:
689 ConP === 0 [[ 821 ]] Oop:java/lang/Stringjava.lang.String
{0x000000010159d7c8} - klass: public final synchronized 'java/lang/String'

  • string: "a"
    :Constant:exact *

After:
689 ConP === 0 [[ 821 ]] Oop:java.lang.String {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String' - string: "a":Constant:exact *

/cc hotspot-compiler


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2178/head:pull/2178
$ git checkout pull/2178

Add a flag _suppress_cr to outputStream. outstream objects won't emit any CR if it's set.
Correct TypeInstPtr::dump2 to make sure it only emits klass name once.
Remove the comment because Klass::oop_print_on() has emitted the address of oop.

Before:
689  ConP  ===  0  [[ 821 ]]   Oop:java/lang/Stringjava.lang.String
{0x000000010159d7c8} - klass: public final synchronized 'java/lang/String'
 - string: "a"
 :Constant:exact *

After:
689  ConP  ===  0  [[ 821 ]]   Oop:java.lang.String {0x000000010159d7c8} - klass: public final synchronized 'java/lang/String' - string: "a":Constant:exact *
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 21, 2021

👋 Welcome back xliu! 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 rfr Pull request is ready for review hotspot-compiler hotspot-compiler-dev@openjdk.org labels Jan 21, 2021
@openjdk
Copy link

openjdk bot commented Jan 21, 2021

@navyxliu
The hotspot-compiler label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Jan 21, 2021

Webrevs

@neliasso
Copy link

neliasso commented Jan 25, 2021

The result your are trying to achieve is good, but I'm not sure pushing supress_cr into outputstream is the right thing. I would like to just not emit the cr's instead - but do also I see that isn't simple, because adding an extra bool to print_on would cascade into the entire codebase.

@navyxliu
Copy link
Member Author

navyxliu commented Jan 28, 2021

@neliasso Thanks for reviewing this.
Exactly. The first reason is I am not familiar with oops/ codebase. I guess some clients expect to see multiple lines. The second reason is that there are many places. I am not sure I can clean them up completely.

That's why I modify outputStream and give it a 'suppress_cr' mode. May I ask hotspot-dev's advice? /cc hotspot-dev

@navyxliu
Copy link
Member Author

navyxliu commented Jan 28, 2021

/cc hotspot-dev

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 28, 2021
@openjdk
Copy link

openjdk bot commented Jan 28, 2021

@navyxliu
The hotspot label was successfully added.

Xin Liu added 2 commits Feb 14, 2021
reimplement this feature. withdraw my intrusive change in outputStream.
use stringStream only for the constant OopPtr. after oop->print_on(st),
delete all appearances of '\n'
@navyxliu
Copy link
Member Author

navyxliu commented Feb 14, 2021

I reimplement this feature using streamStream.

After change, a ConP of an Constant OopPtr becomes a one-liner. eg.

279  ConP  ===  0  [[ 1105 ]]   Oop:java/lang/String java.lang.String {0x000000010100e3d0} - klass: public final synchronized 'java/lang/String' - string: "":Constant:exact *

please note that I keep "Oop:java/lang/String". It's the output klass()->print_name_on(st);
The remaining part "java.lang.String {0x000000010100e3d0} - klass: public final synchronized 'java/lang/String' - string: "":Constant:exact *" is the output of oop->print_on(st). because it's output with :+Verbose, I think it's okay to have a bit verbose.

src/hotspot/share/opto/type.cpp Outdated Show resolved Hide resolved
@navyxliu
Copy link
Member Author

navyxliu commented Feb 20, 2021

/cc hotspot-dev

@openjdk
Copy link

openjdk bot commented Feb 20, 2021

@navyxliu The hotspot label was already applied.

fix build failures on Windows. StringUtils::tr_delete returns size_of.
src/hotspot/share/opto/type.cpp Outdated Show resolved Hide resolved
use the existing api StringUtils::replace_no_expand to archive the same replace.
don't need to invoke os::strdup because stringStream::as_string() has duplicated
the internal buffer.
src/hotspot/share/opto/type.cpp Outdated Show resolved Hide resolved
src/hotspot/share/opto/type.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TobiHartmann TobiHartmann left a comment

That looks good to me.

@openjdk
Copy link

openjdk bot commented Feb 24, 2021

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

8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set

Reviewed-by: thartmann

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

  • 0a4e710: 8261954: Dependencies: Improve iteration over class hierarchy under context class
  • 722142e: 8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java
  • bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
  • fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • ... and 201 more: https://git.openjdk.java.net/jdk/compare/4619f372ae5934091b0d40621a1dbcd9e4b0f80c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 24, 2021
src/hotspot/share/opto/type.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/utilities/test_ostream.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/utilities/test_ostream.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/utilities/test_ostream.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/utilities/test_ostream.cpp Outdated Show resolved Hide resolved
@eastig
Copy link
Member

eastig commented Feb 24, 2021

/label remove ready

@openjdk
Copy link

openjdk bot commented Feb 24, 2021

@eastig Only the PR author and project Committers are allowed to modify labels on a PR.

update comments based on the review feedbacks.
move the unittest to test_stringUtil.cpp.
eastig
eastig approved these changes Feb 25, 2021
@navyxliu
Copy link
Member Author

navyxliu commented Feb 25, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 25, 2021
@openjdk
Copy link

openjdk bot commented Feb 25, 2021

@navyxliu
Your change (at version 2f7ccdb) is now ready to be sponsored by a Committer.

@navyxliu
Copy link
Member Author

navyxliu commented Feb 25, 2021

@eastig Thank you for reviewing it.
@TobiHartmann Could you take a look at it again? I made a little change after you approve it. If everything looks fine, could you sponsor it? Thanks!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

@TobiHartmann
Copy link
Member

TobiHartmann commented Feb 26, 2021

/sponsor

@openjdk openjdk bot closed this Feb 26, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 26, 2021
@openjdk
Copy link

openjdk bot commented Feb 26, 2021

@TobiHartmann @navyxliu Since your change was applied there have been 211 commits pushed to the master branch:

  • 0a4e710: 8261954: Dependencies: Improve iteration over class hierarchy under context class
  • 722142e: 8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java
  • bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
  • fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • ... and 201 more: https://git.openjdk.java.net/jdk/compare/4619f372ae5934091b0d40621a1dbcd9e4b0f80c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7603278.

💡 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
hotspot hotspot-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants