8314029: Add file name parameter to Compiler.perfmap #15871

yftsai · 2023-09-21T20:43:56Z

jcmd Compiler.perfmap uses the hard-coded file name for a perf map: /tmp/perf-%d.map. This change adds an optional argument for specifying a file name.

jcmd PID help Compiler.perfmap shows the following usage.

Compiler.perfmap
Write map file for Linux perf tool.

Impact: Low

Syntax : Compiler.perfmap  [<filename>]

Arguments:
        filename : [optional] Name of the map file (STRING, no default value)

The following section of man page is also updated. (man -l src/jdk.jcmd/share/man/jcmd.1)

       Compiler.perfmap [arguments] (Linux only)
              Write map file for Linux perf tool.

              Impact: Low

              arguments:

              · filename: (Optional) Name of the map file (STRING, no default value)

              If filename is not specified, a default file name is chosen using the pid of the target JVM process.  For example, if the pid is 12345,  then
              the default filename will be /tmp/perf-12345.map.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8314029: Add file name parameter to Compiler.perfmap (Enhancement - P5)
JDK-8316922: Add file name parameter to Compiler.perfmap (CSR) (Withdrawn)

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to dbe223c5
Evgeny Astigeevich (@eastig - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15871

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-21T20:45:18Z

👋 Welcome back yftsai! 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 · 2023-09-21T20:56:41Z

@yftsai The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-09-21T21:00:48Z

Webrevs

dholmes-ora · 2023-09-22T02:48:57Z

src/jdk.jcmd/share/man/jcmd.1

The actual markdown source for this file needs to be updated with these changes. Those sources are not open-source unfortunately. Please either coordinate to get the sources updated with an Oracle developer as part of this PR (they will integrate the internal part), or else please defer this to a subtask and let an Oracle developer update the source and output at the same time. Thanks.

I filed JDK-8320556 to update the closed source. It's assigned to me. I'll do the update after these changes are pushed. In the meantime I'll make sure the current jcmd.1 changes are correct and match the closed changes I'll be making.

I've applied the doc changes in the CSR to our closed markup file and generated a new jcmd.1 file with it. You can apply this diff to this PR to get the open changes checked in:

diff --git a/src/jdk.jcmd/share/man/jcmd.1 b/src/jdk.jcmd/share/man/jcmd.1 index 4157cf600e1..af8d3e61b86 100644 --- a/src/jdk.jcmd/share/man/jcmd.1 +++ b/src/jdk.jcmd/share/man/jcmd.1 @@ -178,11 +178,21 @@ Prints all compiled methods in code cache that are alive. Impact: Medium .RE .TP -\f[V]Compiler.perfmap\f[R] (Linux only) +\f[V]Compiler.perfmap\f[R] [\f[I]arguments\f[R]] (Linux only) Write map file for Linux perf tool. .RS .PP Impact: Low +.PP +\f[I]arguments\f[R]: +.IP \[bu] 2 +\f[V]filename\f[R]: (Optional) Name of the map file (STRING, no default +value) +.PP +If \f[V]filename\f[R] is not specified, a default file name is chosen +using the pid of the target JVM process. +For example, if the pid is \f[V]12345\f[R], then the default +\f[V]filename\f[R] will be \f[V]/tmp/perf-12345.map\f[R]. .RE .TP \f[V]Compiler.queue\f[R]

dholmes-ora · 2023-09-22T02:49:49Z

This will also need a CSR request created and approved.

/csr needed

openjdk · 2023-09-22T02:51:23Z

@dholmes-ora has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@yftsai please create a CSR request for issue JDK-8314029 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

eastig · 2023-09-22T14:11:20Z

src/hotspot/share/code/codeCache.cpp

  // Perf expects to find the map file at /tmp/perf-<pid>.map.
-  char fname[32];
-  jio_snprintf(fname, sizeof(fname), "/tmp/perf-%d.map", os::current_process_id());
+  jio_snprintf(_name, sizeof(_name), "/tmp/perf-%d.map", os::current_process_id());


Please change the comment to:

// Perf expects to find the map file at /tmp/perf-<pid>.map. // It is used as the default file name.

bridgekeeper · 2023-10-20T14:50:06Z

@yftsai 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!

bridgekeeper · 2023-11-17T22:33:21Z

@yftsai This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

plummercj · 2023-11-21T21:31:28Z

@yftsai Since you requested a CSR review, I assume you want this PR re-opened. Can you please re-open it in that case.

yftsai · 2023-11-21T22:14:10Z

/open

openjdk · 2023-11-21T22:15:52Z

@yftsai This pull request is now open

dholmes-ora · 2023-11-30T02:40:29Z

/csr notneeded

openjdk · 2023-11-30T02:40:35Z

@dholmes-ora usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

dholmes-ora · 2023-11-30T02:40:51Z

/csr unneeded

openjdk · 2023-11-30T02:42:31Z

@dholmes-ora The CSR requirement cannot be removed as CSR issues already exist. Please withdraw JDK-8316922 and then use the command /csr unneeded again.

plummercj · 2023-12-05T19:49:28Z

/csr unneeded

openjdk · 2023-12-05T19:50:16Z

@plummercj determined that a CSR request is not needed for this pull request.

plummercj · 2023-12-11T21:39:02Z

/reviewers 2

plummercj

The test needs a copyright update. Otherwise the changes look good.

openjdk · 2023-12-11T21:41:18Z

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

8314029: Add file name parameter to Compiler.perfmap

Reviewed-by: cjplummer, eastigeevich

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

df4ed7e: 8321739: Source launcher fails with "Not a directory" error
5718039: 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes
c516852: 8321889: JavaDoc method references with wrong (nested) type
7d90396: 8321422: Test gc/g1/pinnedobjs/TestPinnedObjectTypes.java times out after completion
6f48240: 8321729: Remove 'orb' field in RMIConnector
e1fd663: 8311306: Test com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java failed: out of expected range
d5214a4: 8321814: G1: Remove unused G1RemSetScanState::_collection_set_iter_state
2611a49: 8321287: Remove unused enum style in Prefetch
b8c0b2f: 8321594: NativeThreadSet should use placeholder for virtual threads
973bcda: 8321631: Fix comments in access.hpp
... and 1295 more: https://git.openjdk.org/jdk/compare/86a18f5e2e0825dddb77656b2f43f64684f1464c...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, @eastig) 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 · 2023-12-11T21:41:27Z

@plummercj
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

eastig

lgtm

yftsai · 2023-12-12T15:54:11Z

/integrate

openjdk · 2023-12-12T15:56:20Z

@yftsai
Your change (at version e1b0b16) is now ready to be sponsored by a Committer.

dean-long · 2023-12-13T20:51:36Z

The man page says "no default value" but then right below describes the default value, which is confusing. I would remove "no default value".
The code already deals with patterns, so why not allow a pattern like /dir/perf-%x.map and document that the platform-specific process id will be passed to String.format() to expand any formatting tokens in the string?

plummercj · 2023-12-13T21:17:58Z

The man page says "no default value" but then right below describes the default value, which is confusing. I would remove "no default value".

This was copied from VM.cds, which pretty much does the same thing (says "no default value" and then explains the default value below). Since the default is based on the pid, and we probably don't want a long description here, maybe just say /tmp/perf-<pid>.map. Or it can say "(STRING, system-generated default name)" as I see in one other jcmd.

phohensee · 2023-12-18T15:20:20Z

/sponsor

openjdk · 2023-12-18T15:21:01Z

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

c0a3b76: 8316197: Make tracing of inline cache available in unified logging
7e1d26d: 8322287: Parallel: Remove unused arg in adjust_eden_for_pause_time and adjust_eden_for_minor_pause_time
5584ba3: 8322097: Serial: Refactor CardTableRS::find_first_clean_card
75d382d: 8322204: Parallel: Remove unused _collection_cost_margin_fraction
febf8af: 8322089: Parallel: Remove PSAdaptiveSizePolicy::set_survivor_size
10335f6: 7001133: OutOfMemoryError by CustomMediaSizeName implementation
ecff9c1: 8315040: Remove redundant check in WorkerPolicy::parallel_worker_threads
a247d0c: 8322209: RISC-V: Enable some tests related to MD5 instrinsic
341b4e0: 8321975: Print when add_reserved_region fails even in product mode
f696796: 8280087: G1: Handle out-of-mark stack situations during reference processing more gracefully
... and 1349 more: https://git.openjdk.org/jdk/compare/86a18f5e2e0825dddb77656b2f43f64684f1464c...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-18T15:21:08Z

@phohensee @yftsai Pushed as commit a5122d7.

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

plummercj · 2023-12-18T17:05:19Z

@phohensee Although this PR had 2 reviews, there was still some unresolved discussion. It should not have been pushed.

phohensee · 2023-12-18T18:54:03Z

Shall I revert it?

plummercj · 2023-12-18T20:30:34Z

If we do settle on some additional changes, I think probably a follow-up CR would be cleaner than a BACKOUT and REDO.

phohensee · 2023-12-18T21:25:16Z

Thanks.

Yi-Fan Tsai added 4 commits September 20, 2023 09:46

8314029: Add file name parameter to Compiler.perfmap

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

c3f02a8

Change to use filename and add a test

b33af2e

Update jcmd man page

fa01b7b

Update parameter name

9fee339

openjdk bot added the rfr label Sep 21, 2023

openjdk bot added serviceability hotspot labels Sep 21, 2023

dholmes-ora reviewed Sep 22, 2023

View reviewed changes

openjdk bot added the csr label Sep 22, 2023

eastig reviewed Sep 22, 2023

View reviewed changes

bridgekeeper bot closed this Nov 17, 2023

Remove changes of jcmd man page

861bda7

openjdk bot reopened this Nov 21, 2023

Yi-Fan Tsai added 3 commits November 21, 2023 23:24

Chagne an option to an argument

61d6f6f

Fix PerfMapTest

0ec20ea

Apply man changes

a7dcf42

openjdk bot removed the csr label Dec 5, 2023

use default argument of write_perf_map

dbe223c

plummercj approved these changes Dec 11, 2023

View reviewed changes

openjdk bot added the ready label Dec 11, 2023

openjdk bot removed the ready label Dec 11, 2023

Update copyright of PerfMapTest

e1b0b16

eastig approved these changes Dec 12, 2023

View reviewed changes

openjdk bot added the ready label Dec 12, 2023

openjdk bot added the sponsor label Dec 12, 2023

openjdk bot added the integrated label Dec 18, 2023

openjdk bot closed this Dec 18, 2023

openjdk bot removed ready rfr sponsor labels Dec 18, 2023

8314029: Add file name parameter to Compiler.perfmap #15871

8314029: Add file name parameter to Compiler.perfmap #15871

Conversation

yftsai commented Sep 21, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

mlbridge bot commented Sep 21, 2023 • edited Loading

Webrevs

dholmes-ora Sep 22, 2023

Choose a reason for hiding this comment

plummercj Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

plummercj Nov 28, 2023

Choose a reason for hiding this comment

dholmes-ora commented Sep 22, 2023

openjdk bot commented Sep 22, 2023

eastig Sep 22, 2023

Choose a reason for hiding this comment

bridgekeeper bot commented Oct 20, 2023

bridgekeeper bot commented Nov 17, 2023

plummercj commented Nov 21, 2023

yftsai commented Nov 21, 2023

openjdk bot commented Nov 21, 2023

dholmes-ora commented Nov 30, 2023

openjdk bot commented Nov 30, 2023

dholmes-ora commented Nov 30, 2023

openjdk bot commented Nov 30, 2023

plummercj commented Dec 5, 2023

openjdk bot commented Dec 5, 2023

plummercj commented Dec 11, 2023

plummercj left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 11, 2023 • edited Loading

openjdk bot commented Dec 11, 2023

eastig left a comment

Choose a reason for hiding this comment

yftsai commented Dec 12, 2023

openjdk bot commented Dec 12, 2023

dean-long commented Dec 13, 2023

plummercj commented Dec 13, 2023 • edited Loading

phohensee commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

openjdk bot commented Dec 18, 2023

plummercj commented Dec 18, 2023

phohensee commented Dec 18, 2023

plummercj commented Dec 18, 2023

phohensee commented Dec 18, 2023

yftsai commented Sep 21, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 21, 2023 •

edited

Loading

plummercj Nov 21, 2023 •

edited

Loading

openjdk bot commented Dec 11, 2023 •

edited

Loading

plummercj commented Dec 13, 2023 •

edited

Loading