8261441: JFR: Filename expansion #4550

D-D-H · 2021-06-22T09:06:13Z

Hi,
Could I have a review of this change that let the users could use %p in the filename to represent the PID when JFR.start/stop/dump.

~~I haven't implemented %t described in the issue, because I don't think it's very useful.~~

Best,
Denghui Dong

Progress

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

Issue

JDK-8261441: JFR: Filename expansion

Reviewers

Jaroslav Bachorik (@jbachorik - Reviewer) ⚠️ Review applies to a9f62c7
Erik Gahlin (@egahlin - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4550

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

Using diff file

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

D-D-H · 2021-06-22T09:06:23Z

/label add hotspot-jfr

bridgekeeper · 2021-06-22T09:07:05Z

👋 Welcome back ddong! 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 · 2021-06-22T09:09:25Z

@D-D-H
The hotspot-jfr label was successfully added.

mlbridge · 2021-06-22T09:12:47Z

Webrevs

egahlin · 2021-06-22T10:14:56Z

/csr

openjdk · 2021-06-22T10:15:32Z

@egahlin has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@D-D-H please create a CSR request for issue JDK-8261441. This pull request cannot be integrated until the CSR request is approved.

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdDump.java

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdStart.java

egahlin · 2021-06-23T03:17:36Z

Why is not %t useful?

If you start multiple processes, the same PID can be reused and cause a collision.

D-D-H · 2021-06-23T08:03:03Z

Why is not %t useful?

If you start multiple processes, the same PID can be reused and cause a collision.

Makes sense.
%t support is added.

Thanks

D-D-H · 2021-06-23T08:26:18Z

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java

+                    i++;
+                } else if (nc == 't') {
+                    if (time == null) {
+                        time = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss").format(new Date());


I use the current time here so that different filenames could be generated for multiple dumps of one process.

There is a method, jdk.internal.Utils::formatDateTime, which is used to generate a filename if a directory is specified (instead of file). It would be good if the same format and method could be used with '%t'. That method would avoid loading the SimpleDateFormat class and other time/formatting related classes.

Good suggestion.
Fixed.

bridgekeeper · 2021-07-21T12:54:10Z

@D-D-H This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2021-07-25T19:37:42Z

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

8261441: JFR: Filename expansion

Reviewed-by: jbachorik, egahlin

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

3ab95d1: 8271905: mark hotspot runtime/Metaspace tests which ignore external VM flags
e2c5bfe: 8271308: (fc) FileChannel.transferTo() transfers no more than Integer.MAX_VALUE bytes in one call
7234a43: 8271953: fix mis-merge in JDK-8271878
d7fc9e4: 8267840: Improve URLStreamHandler.parseURL()
55bd52a: 8271840: Add simple Integer.toString microbenchmarks
18dd4d4: 8271121: ZGC: stack overflow (segv) when -Xlog:gc+start=debug
685fc3c: 8270903: sun.net.httpserver.HttpConnection: Improve toString
4abe531: 8271722: [TESTBUG] gc/g1/TestMixedGCLiveThreshold.java can fail if G1 Full GC uses >1 workers
ea9a595: 8270058: Use Objects.check{Index,FromIndexSize} for java.desktop
64d18d4: 4819544: SwingSet2 JTable Demo throws NullPointerException
... and 583 more: https://git.openjdk.java.net/jdk/compare/0458113c6b1cf500ffdf049c1e3a698b16ce12ce...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 (@jbachorik, @egahlin) 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).

D-D-H · 2021-07-26T02:58:29Z

@egahlin
Thank you very much for helping to refine the content of CSR.
I have updated the corresponding man page.

D-D-H · 2021-08-04T02:35:27Z

@egahlin
Hi Erik,
Could you help review this pr?

egahlin · 2021-08-05T00:28:25Z

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/AbstractDCmd.java

@@ -191,7 +192,7 @@ protected final void print(String s) {
    }

    protected final void print(String s, Object... args) {
-        currentLine.append(String.format(s, args));
+        currentLine.append(args != null && args.length > 0 ? String.format(s, args) : s);


Why is args != null check needed?

(Fail fast may be preferable if something unexpected happens)

This check is not necessary(args is not null even if no any arg passes in), removed.

D-D-H · 2021-08-05T16:36:42Z

/integrate

openjdk · 2021-08-05T16:38:11Z

@D-D-H
Your change (at version fbde563) is now ready to be sponsored by a Committer.

y1yang0 · 2021-08-06T02:10:36Z

/sponsor

openjdk · 2021-08-06T02:12:09Z

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

e38e365: 8271208: Typo in ModuleDescriptor.read javadoc
14692d5: Merge
dfacda4: 8270872: Final nroff manpage update for JDK 17
90f85ff: 8271588: JFR Recorder Thread crashed with SIGSEGV in write_klass
f312f28: 8271863: ProblemList serviceability/sa/TestJmapCore.java on linux-x64 with ZGC
62e72ad: 8271293: Monitor class should use ThreadBlockInVMPreprocess
cb36880: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS
3ab95d1: 8271905: mark hotspot runtime/Metaspace tests which ignore external VM flags
e2c5bfe: 8271308: (fc) FileChannel.transferTo() transfers no more than Integer.MAX_VALUE bytes in one call
7234a43: 8271953: fix mis-merge in JDK-8271878
... and 590 more: https://git.openjdk.java.net/jdk/compare/0458113c6b1cf500ffdf049c1e3a698b16ce12ce...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-06T02:12:24Z

@kelthuzadx @D-D-H Pushed as commit adb0ae5.

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

8261441: JFR: Filename expansion

57337ae

openjdk bot added rfr Pull request is ready for review hotspot-jfr hotspot-jfr-dev@openjdk.org labels Jun 22, 2021

Updates test

7c4146d

openjdk bot added the csr Pull request needs approved CSR before integration label Jun 22, 2021

fix format problem

f3fed5a

jbachorik reviewed Jun 22, 2021

View reviewed changes

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdDump.java Outdated Show resolved Hide resolved

jbachorik reviewed Jun 22, 2021

View reviewed changes

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdStart.java Outdated Show resolved Hide resolved

Hamlin-Li mentioned this pull request Jun 23, 2021

JDK-8269202: Support %p in jfr dump file name #4564

Closed

3 tasks

D-D-H added 2 commits June 23, 2021 10:45

fix print problem

c69da27

null check

004270d

D-D-H added 2 commits June 23, 2021 15:35

%t support

ff31746

update

a9f62c7

D-D-H commented Jun 23, 2021

View reviewed changes

jbachorik approved these changes Jun 23, 2021

View reviewed changes

D-D-H added 2 commits June 23, 2021 18:52

update

eb072e2

update

07525b3

openjdk bot removed the csr Pull request needs approved CSR before integration label Jul 25, 2021

openjdk bot added the ready Pull request is ready to be integrated label Jul 25, 2021

update man page

95b1264

egahlin reviewed Aug 5, 2021

View reviewed changes

remove null check

fbde563

egahlin approved these changes Aug 5, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 5, 2021

openjdk bot closed this Aug 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8261441: JFR: Filename expansion #4550

8261441: JFR: Filename expansion #4550

D-D-H commented Jun 22, 2021 •

edited by openjdk bot

Loading

D-D-H commented Jun 22, 2021

bridgekeeper bot commented Jun 22, 2021

openjdk bot commented Jun 22, 2021

mlbridge bot commented Jun 22, 2021 •

edited

Loading

egahlin commented Jun 22, 2021

openjdk bot commented Jun 22, 2021

egahlin commented Jun 23, 2021

D-D-H commented Jun 23, 2021 •

edited

Loading

D-D-H Jun 23, 2021 •

edited

Loading

egahlin Jun 23, 2021 •

edited

Loading

D-D-H Jun 23, 2021

bridgekeeper bot commented Jul 21, 2021

openjdk bot commented Jul 25, 2021 •

edited

Loading

D-D-H commented Jul 26, 2021

D-D-H commented Aug 4, 2021

egahlin Aug 5, 2021

D-D-H Aug 5, 2021

D-D-H commented Aug 5, 2021

openjdk bot commented Aug 5, 2021

y1yang0 commented Aug 6, 2021

openjdk bot commented Aug 6, 2021

openjdk bot commented Aug 6, 2021

8261441: JFR: Filename expansion #4550

8261441: JFR: Filename expansion #4550

Conversation

D-D-H commented Jun 22, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

D-D-H commented Jun 22, 2021

bridgekeeper bot commented Jun 22, 2021

openjdk bot commented Jun 22, 2021

mlbridge bot commented Jun 22, 2021 • edited Loading

Webrevs

egahlin commented Jun 22, 2021

openjdk bot commented Jun 22, 2021

egahlin commented Jun 23, 2021

D-D-H commented Jun 23, 2021 • edited Loading

D-D-H Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

egahlin Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

D-D-H Jun 23, 2021

Choose a reason for hiding this comment

bridgekeeper bot commented Jul 21, 2021

openjdk bot commented Jul 25, 2021 • edited Loading

D-D-H commented Jul 26, 2021

D-D-H commented Aug 4, 2021

egahlin Aug 5, 2021

Choose a reason for hiding this comment

D-D-H Aug 5, 2021

Choose a reason for hiding this comment

D-D-H commented Aug 5, 2021

openjdk bot commented Aug 5, 2021

y1yang0 commented Aug 6, 2021

openjdk bot commented Aug 6, 2021

openjdk bot commented Aug 6, 2021

D-D-H commented Jun 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 22, 2021 •

edited

Loading

D-D-H commented Jun 23, 2021 •

edited

Loading

D-D-H Jun 23, 2021 •

edited

Loading

egahlin Jun 23, 2021 •

edited

Loading

openjdk bot commented Jul 25, 2021 •

edited

Loading