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-8261095: Add test for clhsdb "symbol" command #2863

Closed
wants to merge 5 commits into from

Conversation

@Vipin-Sharma
Copy link
Contributor

@Vipin-Sharma Vipin-Sharma commented Mar 7, 2021


Progress

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

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 7, 2021

👋 Welcome back vsharma! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 7, 2021

@Vipin-Sharma The following label will be automatically applied to this pull request:

  • serviceability

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.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 7, 2021

Loading

@Vipin-Sharma Vipin-Sharma changed the title JDK-8261095: Add test for clhsdb symbol command JDK-8261095: Add test for clhsdb "symbol" command Mar 7, 2021
Copy link
Contributor

@plummercj plummercj left a comment

Overall it looks good. I have some suggestions for improved comments and formatting, and also please use the line.separator property.

Loading

theApp = LingeredApp.startApp();
System.out.println("Started LingeredApp with pid " + theApp.getPid());

//Use command "class java.lang.Thread" to get the address of the InstanceKlass for java.lang.Thread
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

Put a space after the //. Same goes for all the comments below.

Loading

//extract address comes along with Symbol instance, following is corresponding sample output line
//Symbol* Klass::_name: Symbol @ 0x0000000800471120
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

// The inspect command output will have one line of output that looks like the following.
// Symbol* Klass::_name: Symbol @ 0x0000000800471120
// Extract the Symbol address from it.

Loading

throw new RuntimeException("Cannot find address with Symbol instance");
}

//Running "symbol" command on the Symbol instance address extracted in previous step
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

// Run "symbol" command on the Symbol instance address extracted in previous step.
// It should produce the symbol for java/lang/Thread.

Loading

expStrMap = new HashMap<>();
expStrMap.put(cmdStr, List.of("#java/lang/Thread"));
test.run(theApp.getPid(), cmds, expStrMap, null);

Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

No need for newline here.

Loading

String threadAddress = null;
String[] parts = classOutput.split("\n");

//extract thread address from the output line similar to "java/lang/Thread @0x000000080001d940"
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

I'd prefer:

// Extract the thread InstanceKlass address from the output, which looks similar to the following:
//   java/lang/Thread @0x000000080001d940

Loading

expStrMap.put(cmdStr, List.of("java/lang/Thread"));
String classOutput = test.run(theApp.getPid(), cmds, expStrMap, null);
String threadAddress = null;
String[] parts = classOutput.split("\n");
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

I think you should be using the line.separator properly instead of "\n".
String linesep = System.getProperty("line.separator");

Loading

Copy link
Member

@YaSuenag YaSuenag Mar 9, 2021

Choose a reason for hiding this comment

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

It might be more simple if you use String::lines and stream API.

Loading

Copy link
Contributor

@plummercj plummercj Mar 9, 2021

Choose a reason for hiding this comment

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

Yes. Coincidentally I was just having an internal chat about split(linesep), and String.lines() was mentioned. We have a lot of existing code that uses split(linesep) because String.lines() has only been around since JDK 11, and even some newer test code we have uses split(linsep) because it was copied from older tests.

Loading

expStrMap.put(cmdStr, List.of("Symbol"));
String inspectOutput = test.run(theApp.getPid(), cmds, expStrMap, null);
String symbolAddress = null;
parts = inspectOutput.split("\n");
Copy link
Contributor

@plummercj plummercj Mar 8, 2021

Choose a reason for hiding this comment

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

And here also use line.separator.

Loading

.map(part -> part.split(" @"))
.findFirst()
.map(addresses -> addresses[1])
.orElse(null);
Copy link
Member

@YaSuenag YaSuenag Mar 9, 2021

Choose a reason for hiding this comment

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

It should just call get() because NoSuchElementException would be called when we cannot find out Thread class from stdout.
If you want to throw RuntimeException like next step, you should use orElseThrow().

Loading

String symbolAddress = inspectOutput.lines().filter(part -> part.startsWith("Symbol"))
.map(part -> part.split("@ "))
.findFirst().map(symbolParts -> symbolParts[1])
.orElse(null);
Copy link
Member

@YaSuenag YaSuenag Mar 9, 2021

Choose a reason for hiding this comment

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

Same comment in above.

Loading

.orElseThrow(() -> new RuntimeException("Cannot find address of " +
"the InstanceKlass for java.lang.Thread in output"));
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

These lines really don't format well, a reason why the previous orElse() version is probably better. Maybe you can make this work by moving the .filter() call to a new line that is indented 8 more than the previous line

Loading

Copy link
Contributor Author

@Vipin-Sharma Vipin-Sharma Mar 10, 2021

Choose a reason for hiding this comment

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

After moving the filter to the next line also can not fit this in one line.
In addition to the filter, moving all characters starting from "new RuntimeException(...." also goes till 126 char.
Following is one suggestion, does it look good?

String threadAddress = classOutput.lines()
.filter(part -> part.startsWith("java/lang/Thread"))
.map(part -> part.split(" @"))
.findFirst()
.map(addresses -> addresses[1])
.orElseThrow(() -> new RuntimeException(
"Cannot find address of the InstanceKlass for java.lang.Thread in output"));

Loading

Copy link
Contributor Author

@Vipin-Sharma Vipin-Sharma Mar 10, 2021

Choose a reason for hiding this comment

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

Formatting in my comment is not clear, so I have added one new commit for this.

Loading

Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

Formatting looked fine in the email. Try putting it in a code block to prevent the auto formatting being done on it.

Latest webrev looks good.

Loading

"the InstanceKlass for java.lang.Thread in output"));


// Use "inspect" on the thread address we extracted in previous step
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

"in the previous step"

Loading

Copy link
Contributor

@plummercj plummercj left a comment

I didn't realize that lines() returns a Stream. TBH I find the original code a lot easier to read than the Stream code, but that's pretty much always my opinion when I see Stream code. Others will probably feel your new version is more elegant, so I won't protest.

Having said that, I also just learned that String.split("\R") will properly split the lines with any newline character sequence. See Pattern:
\R Any Unicode linebreak sequence \u000D\u000A|[\u000A\u000B\u000C\u000D\u0085\u2028\u2029]

Loading

String inspectOutput = test.run(theApp.getPid(), cmds, expStrMap, null);

// The inspect command output will have one line of output that looks like the following.
// Symbol* Klass::_name: Symbol @ 0x0000000800471120
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

Indent this line of the comment

Loading

.orElseThrow(() -> new RuntimeException(
"Cannot find address with Symbol instance"));

// Run "symbol" command on the Symbol instance address extracted in previous step.
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

"in the previous step"

Loading

.orElseThrow(() -> new RuntimeException(
"Cannot find address with Symbol instance"));
Copy link
Contributor

@plummercj plummercj Mar 10, 2021

Choose a reason for hiding this comment

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

Same issue with formatting as above.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

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

8261095: Add test for clhsdb "symbol" command

Reviewed-by: cjplummer, ysuenaga

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

  • 67ea3bd: 8263102: Expand documention of Method.isBridge
  • d0c1aec: 8263140: Japanese chars garble in console window in HSDB
  • 70342e8: 8262520: Add SA Command Line Debugger support to connect to debug server
  • e5ce97b: 8263206: assert(*error_msg != '\0') failed: Must have error_message while parsing -XX:CompileCommand=unknown
  • 3212f80: 8261937: LambdaForClassInBaseArchive: SimpleApp$$Lambda$1 missing
  • 2218e72: 8262486: Merge trivial JDWP agent changes from the loom repo to the jdk repo
  • 86fac95: 8263142: Delete unused entry points in libawt/libawt_xawt/libawt_headless
  • b7f0b3f: 8252173: Use handles instead of jobjects in modules.cpp
  • a6e34b3: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()
  • fbe40e8: 8252399: Update mapMulti documentation to use type test pattern instead of instanceof once JEP 375 exits preview
  • ... and 26 more: https://git.openjdk.java.net/jdk/compare/18fc35053c2363686d6ecae44ff2d4cce791eef8...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 (@plummercj, @YaSuenag) 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).

Loading

@openjdk openjdk bot added the ready label Mar 10, 2021
@Vipin-Sharma
Copy link
Contributor Author

@Vipin-Sharma Vipin-Sharma commented Mar 10, 2021

/integrate

Loading

@openjdk openjdk bot added the sponsor label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@Vipin-Sharma
Your change (at version c977ed1) is now ready to be sponsored by a Committer.

Loading

@Vipin-Sharma
Copy link
Contributor Author

@Vipin-Sharma Vipin-Sharma commented Mar 17, 2021

This pull request is reviewed and ready to be integrated, please sponsor the request.

Loading

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 17, 2021

Ok, I will sponsor you.

Loading

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Mar 17, 2021

/sponsor

Loading

@openjdk openjdk bot closed this Mar 17, 2021
@openjdk openjdk bot removed the rfr label Mar 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2021

@YaSuenag @Vipin-Sharma Since your change was applied there have been 140 commits pushed to the master branch:

  • ec95a5c: 8263410: ListModel javadoc refers to non-existent interface
  • 7b9d256: 8261262: Kitchensink24HStress.java crashed with EXCEPTION_ACCESS_VIOLATION
  • d2144a5: 8263058: Optimize vector shift with zero shift count
  • dd6c911: 8263705: Two shenandoah tests fail due to can't find ClassFileInstaller
  • 4acb883: 8261666: [mlvm] Remove WhiteBoxHelper
  • 5069796: 8263164: assert(_base >= VectorA && _base <= VectorZ) failed: Not a Vector while calling StoreVectorNode::memory_size()
  • 996079b: 8260650: test failed with "assert(false) failed: infinite loop in PhaseIterGVN::optimize"
  • 9cb9af6: 8260959: remove RECORDS from PreviewFeature.Feature enum
  • 05fe06a: 8255790: GTKL&F: Java 16 crashes on initialising GTKL&F on Manjaro Linux
  • 422eba8: 8263536: Add @build tags to jpackage tests
  • ... and 130 more: https://git.openjdk.java.net/jdk/compare/18fc35053c2363686d6ecae44ff2d4cce791eef8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 086a66a.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants