8289780: Avoid formatting stub names when Forte is not enabled #9386

iklam · 2022-07-06T00:38:14Z

Forte::register_stub() should be called only when the JVm is being instrumented by Forte (aka "Oracle Developer Studio")

https://www.oracle.com/tools/developerstudio/downloads/developer-studio-jsp.html

We currently always format the name of generated stubs and call Forte::register_stub(), which usually does nothing.

Example:

https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/sharedRuntime.cpp#L2686-L2697

To improve start-up, we should check if Forte is enabled before formatting the name.

I also renamed some #ifndef IA64 around the code that I touched.

Progress

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

Issue

JDK-8289780: Avoid formatting stub names when Forte is not enabled

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to a1523c9d
Coleen Phillimore (@coleenp - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9386

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

Using diff file

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

bridgekeeper · 2022-07-06T00:39:26Z

👋 Welcome back iklam! 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 · 2022-07-06T00:41:29Z

@iklam 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 · 2022-07-06T01:07:03Z

Webrevs

01: Full - Incremental (609350f1)
00: Full (a1523c9d)

dholmes-ora

Cleanup looks good! Thanks.

openjdk · 2022-07-06T01:25:17Z

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

8289780: Avoid formatting stub names when Forte is not enabled

Reviewed-by: dholmes, coleenp, sspitsyn

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

54b4576: 8288699: cleanup HTML tree in HtmlDocletWriter.commentTagsToContent
6aaf141: 8289984: Files:isDirectory and isRegularFile methods not throwing SecurityException
1877533: 6522064: Aliases from Microsoft CryptoAPI has bad character encoding
9c86c82: 8282714: synthetic arguments are being added to the constructors of static local classes
e779585: 8271707: migrate tests to use jdk.test.whitebox.WhiteBox
d852e99: 8289697: buffer overflow in MTLVertexCache.m: MTLVertexCache_AddGlyphQuad
a13af65: 8282322: AArch64: Provide a means to eliminate all STREX family of instructions
f1967cf: 8289997: gc/g1/TestVerificationInConcurrentCycle.java fails due to use of debug-only option
1b8f466: 8289740: Add verification testing during all concurrent phases in G1
ac399e9: 8286957: Held monitor count
... and 62 more: https://git.openjdk.org/jdk/compare/77c3bbf105403089fec69d51406fe3e6f562271f...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.

coleenp

This looks good. Leaving the name forte.cpp seems odd, even though some still refer to it that way. My vote would have been for asyncGetCallTrace.cpp

iklam · 2022-07-06T17:49:46Z

It turns out that collector_func_load is used by OracleDeveloperStudio on Linux as well, so I can't remove the call to collector_func_load. I think this (weak) function will be non-null when OracleDeveloperStudio loads the JVM:

$ nm OracleDeveloperStudio12.6-linux-x86-bin/developerstudio12.6/lib/analyzer/amd64/libcollector.so \
     | grep collector_func_load
000000000002efd0 T __collector_func_load
000000000002efd0 W collector_func_load

So slight change of plan. I am adding a check to avoid computing the blob_id, etc, when Forte::is_enabled()` is false, which will be the case in most cases.

coleenp · 2022-07-06T19:09:09Z

src/hotspot/share/prims/forte.hpp


 class Forte : AllStatic {
 public:
+   static bool is_enabled() NOT_JVMTI_RETURN_(false);


I don't think the rest of this forte code is disabled by JVMTI.

If the answer to whether it's enabled is something you want to be fast, and doesn't change, maybe make it check a variable?

The code in the Forte class, as well as the non-trivial of AsyncGetCallTrace() in forte.cpp, are inside #if INCLUDE_JVMTI. That's why I use NOT_JVMTI_RETURN_ for the new function.

Ok, thanks for answering my question.

sspitsyn

I've posed one comment.
Other than that the fix looks okay.
Thanks,
Serguei

sspitsyn · 2022-07-08T22:53:49Z

src/hotspot/share/runtime/sharedRuntime.cpp

+
+    if (JvmtiExport::should_post_dynamic_code_generated()) {
+      JvmtiExport::post_dynamic_code_generated(blob_id, new_adapter->content_begin(), new_adapter->content_end());
+    }


The lines 2698-2670 is better to move out of the if-statement at the line 2687.

HI Serguei, thanks for the review. Lines 2698-2700 need the blob_id which is generated at line 2688, so they have to stay inside the outer "if" block.

Thanks, Ioi. You are right.

iklam · 2022-07-09T03:46:23Z

Thanks @dholmes-ora, @sspitsyn, @coleenp for the review.
/integrate

openjdk · 2022-07-09T03:47:32Z

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

54b4576: 8288699: cleanup HTML tree in HtmlDocletWriter.commentTagsToContent
6aaf141: 8289984: Files:isDirectory and isRegularFile methods not throwing SecurityException
1877533: 6522064: Aliases from Microsoft CryptoAPI has bad character encoding
9c86c82: 8282714: synthetic arguments are being added to the constructors of static local classes
e779585: 8271707: migrate tests to use jdk.test.whitebox.WhiteBox
d852e99: 8289697: buffer overflow in MTLVertexCache.m: MTLVertexCache_AddGlyphQuad
a13af65: 8282322: AArch64: Provide a means to eliminate all STREX family of instructions
f1967cf: 8289997: gc/g1/TestVerificationInConcurrentCycle.java fails due to use of debug-only option
1b8f466: 8289740: Add verification testing during all concurrent phases in G1
ac399e9: 8286957: Held monitor count
... and 62 more: https://git.openjdk.org/jdk/compare/77c3bbf105403089fec69d51406fe3e6f562271f...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-09T03:47:46Z

@iklam Pushed as commit 3c08e6b.

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

8289780: Remove Forte::register_stub

a1523c9

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jul 6, 2022

iklam marked this pull request as ready for review July 6, 2022 01:01

openjdk bot added the rfr Pull request is ready for review label Jul 6, 2022

dholmes-ora approved these changes Jul 6, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 6, 2022

coleenp approved these changes Jul 6, 2022

View reviewed changes

Do not remove Forte::register_stub as it is used on Linux as well

609350f

iklam changed the title ~~8289780: Remove Forte::register_stub~~ 8289780: Avoid formatting stub names when Forte is not enabled Jul 6, 2022

coleenp reviewed Jul 6, 2022

View reviewed changes

coleenp approved these changes Jul 8, 2022

View reviewed changes

sspitsyn approved these changes Jul 8, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 9, 2022

openjdk bot closed this Jul 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 9, 2022

8289780: Avoid formatting stub names when Forte is not enabled #9386

8289780: Avoid formatting stub names when Forte is not enabled #9386

Uh oh!

Conversation

iklam commented Jul 6, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 6, 2022

Uh oh!

openjdk bot commented Jul 6, 2022

Uh oh!

mlbridge bot commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

iklam commented Jul 6, 2022

Uh oh!

coleenp Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

coleenp Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

iklam Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

coleenp Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

iklam Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

sspitsyn Jul 9, 2022

Choose a reason for hiding this comment

Uh oh!

iklam commented Jul 9, 2022

Uh oh!

openjdk bot commented Jul 9, 2022

Uh oh!

openjdk bot commented Jul 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

iklam commented Jul 6, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 6, 2022 •

edited

Loading

openjdk bot commented Jul 6, 2022 •

edited

Loading