8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads #7555

shipilev · 2022-02-21T11:22:19Z

In several places in hotspot, we call CompileBroker::log_metaspace_failure(). I believe the intent is to call it from compiler threads only, because it ends up calling:

  void log_metaspace_failure(const char* reason) {
    ResourceMark rm;
    StringLogMessage lm;
    lm.print("%4d COMPILE PROFILING SKIPPED: %s", -1, reason);
    lm.print("\n");
    log(JavaThread::current(), "%s", (const char*)lm);
  }

...which would fail the cast in JavaThread::current() if current thread is not Java thread (compiler threads are Java threads too).

It clearly manifests on JDK-8282170 test:

12700 breakpoints set
12800 breakpoints set
12900 breakpoints set
13000 breakpoints set
# To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/thread.hpp:1458
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/home/shade/trunks/jdk/src/hotspot/share/runtime/thread.hpp:1458), pid=889379, tid=889389
# assert(t->is_Java_thread()) failed: incorrect cast to JavaThread
V [libjvm.so+0xacfad4] CompilationLog::log_metaspace_failure(char const*)+0x494
V [libjvm.so+0xac84b0] CompileBroker::log_metaspace_failure()+0x20
V [libjvm.so+0x15069e9] Method::build_method_counters(Thread*, Method*)+0x119
V [libjvm.so+0x150851b] BreakpointInfo::set(Method*)+0xfb
V [libjvm.so+0x127d5ea] JvmtiBreakpoint::each_method_version_do(void (Method::*)(int))+0x5a
V [libjvm.so+0x1b342ae] VM_Operation::evaluate()+0x18e

Additional testing:

Linux x86_64 fastdebug does not fail JDK-8282170 test anymore

Progress

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

Issue

JDK-8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7555

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

Using diff file

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

bridgekeeper · 2022-02-21T11:22:30Z

👋 Welcome back shade! 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-02-21T11:25:05Z

@shipilev The following label will be automatically applied to this pull request:

hotspot-compiler

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.

mlbridge · 2022-02-21T11:28:13Z

Webrevs

01: Full - Incremental (8a91ce8d)
00: Full (82508eb5)

vnkozlov

No, it could be called from Java execution thread too when profiling is triggered. But yes it was assumed that only Java thread can ask for creation MethodData.
On other hand log() methods accepts Thread* parameter and just record it:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/utilities/events.hpp#L183
May be we don't need to cast it to JavaThread in this case.

shipilev · 2022-02-23T07:36:16Z

No, it could be called from Java execution thread too when profiling is triggered. But yes it was assumed that only Java thread can ask for creation MethodData. On other hand log() methods accepts Thread* parameter and just record it: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/utilities/events.hpp#L183 May be we don't need to cast it to JavaThread in this case.

Yes, this would also work:

diff --git a/src/hotspot/share/compiler/compileBroker.cpp b/src/hotspot/share/compiler/compileBroker.cpp
index 1c8656044b5..62c45e6adca 100644
--- a/src/hotspot/share/compiler/compileBroker.cpp
+++ b/src/hotspot/share/compiler/compileBroker.cpp
@@ -227,11 +227,11 @@ class CompilationLog : public StringEventLog {
   void log_metaspace_failure(const char* reason) {
     ResourceMark rm;
     StringLogMessage lm;
     lm.print("%4d   COMPILE PROFILING SKIPPED: %s", -1, reason);
     lm.print("\n");
-    log(JavaThread::current(), "%s", (const char*)lm);
+    log(Thread::current(), "%s", (const char*)lm);
   }
 };
 
 static CompilationLog* _compilation_log = NULL;

...but it is odd to: a) have the rest of CompilationLog accept JavaThread only; b) having COMPILE SKIPPED printed when we are not dealing with any compiler/Java thread, for example from VMThread / JVMTI code like on the path in the PR body.

void CompileBroker::log_metaspace_failure() {
  const char* message = "some methods may not be compiled because metaspace "
                        "is out of memory";
  if (_compilation_log != NULL) {
    _compilation_log->log_metaspace_failure(message);
  }
  if (PrintCompilation) {
    tty->print_cr("COMPILE PROFILING SKIPPED: %s", message); // <--- from here
  }
}

So I still believe filtering non-Java/compiler threads at CompileBroker::log_metaspace_failure() is the proper fix.

src/hotspot/share/compiler/compileBroker.cpp

vnkozlov

Good.

openjdk · 2022-02-23T17:12:24Z

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

8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads

Reviewed-by: kvn, 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 45 new commits pushed to the master branch:

cd36be4: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
bf19fc6: 8280357: user.home = "?" when running with systemd DynamicUser=true
b6843a1: 8005885: enhance PrintCodeCache to print more data
23995f8: 8281525: Enable Zc:strictStrings flag in Visual Studio build
20e78f7: 8282307: Parallel: Incorrect discovery mode in PCReferenceProcessor
0b6862e: 8282348: Remove unused CardTable::dirty_card_iterate
6fab8a2: 8277204: Implement PAC-RET branch protection on Linux/AArch64
abc0ce1: 8282316: Operation before String case conversion
0796620: 8281944: JavaDoc throws java.lang.IllegalStateException: ERRONEOUS
231e48f: 8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422
... and 35 more: https://git.openjdk.java.net/jdk/compare/e0b49629e95c98aabe8b75ec2f7528e7fb6dcffc...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.

TobiHartmann

Looks good to me too.

tstuefe · 2022-02-25T09:26:36Z

AFAICS FormatStringEventLog::log() - which is only used from the compiler btw - is always called with the current thread. Maybe it should just use Thread::current instead?

shipilev · 2022-02-28T13:12:31Z

AFAICS FormatStringEventLog::log() - which is only used from the compiler btw - is always called with the current thread. Maybe it should just use Thread::current instead?

Maybe. Let's do that separately?

tstuefe · 2022-02-28T13:23:16Z

AFAICS FormatStringEventLog::log() - which is only used from the compiler btw - is always called with the current thread. Maybe it should just use Thread::current instead?

Maybe. Let's do that separately?

Oh, sure.

shipilev · 2022-02-28T13:28:25Z

Good. Thanks for reviews!

/integrate

openjdk · 2022-02-28T13:29:33Z

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

0ae3d1d: 8282131: java.time.ZoneId should be a sealed abstract class
c58f5c6: 8282360: Merge POSIX implementations of ThreadCritical
06cadb3: 8230382: Clean up ConvI2L, CastII and CastLL::Ideal methods
efd3967: 8267265: Use new IR Test Framework to create tests for C2 Ideal transformations
86723d4: 8281507: Two javac tests have bad jtreg @clean tags
630ad1a: 8282428: ProblemList jdk/jfr/jvm/TestWaste.java
afd4bcb: 8282398: EndingDotHostname.java test fails because SSL cert expired
cf6d256: 8282153: JFR: Check for recording waste
c5c6058: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX
fb8bf81: 8279995: jpackage --add-launcher option should allow overriding description
... and 51 more: https://git.openjdk.java.net/jdk/compare/e0b49629e95c98aabe8b75ec2f7528e7fb6dcffc...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-28T13:29:47Z

@shipilev Pushed as commit 4e7fb41.

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

Fix

82508eb

openjdk bot added the rfr Pull request is ready for review label Feb 21, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 21, 2022

vnkozlov suggested changes Feb 22, 2022

View reviewed changes

Merge branch 'master' into JDK-8282172-compiler-broker-message

10bdbc1

shipilev changed the title ~~8282172: CompileBroker::log_metaspace_failure is called from non-compiler threads~~ 8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads Feb 23, 2022

vnkozlov reviewed Feb 23, 2022

View reviewed changes

src/hotspot/share/compiler/compileBroker.cpp Outdated Show resolved Hide resolved

Another fix

8a91ce8

vnkozlov approved these changes Feb 23, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 23, 2022

TobiHartmann approved these changes Feb 25, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 28, 2022

openjdk bot closed this Feb 28, 2022

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

shipilev deleted the JDK-8282172-compiler-broker-message branch March 7, 2022 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads #7555

8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads #7555

shipilev commented Feb 21, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

mlbridge bot commented Feb 21, 2022 •

edited

Loading

vnkozlov left a comment

shipilev commented Feb 23, 2022 •

edited

Loading

vnkozlov left a comment

openjdk bot commented Feb 23, 2022 •

edited

Loading

TobiHartmann left a comment

tstuefe commented Feb 25, 2022

shipilev commented Feb 28, 2022

tstuefe commented Feb 28, 2022

shipilev commented Feb 28, 2022

openjdk bot commented Feb 28, 2022

openjdk bot commented Feb 28, 2022

8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads #7555

8282172: CompileBroker::log_metaspace_failure is called from non-Java/compiler threads #7555

Conversation

shipilev commented Feb 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

mlbridge bot commented Feb 21, 2022 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

shipilev commented Feb 23, 2022 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 23, 2022 • edited Loading

TobiHartmann left a comment

Choose a reason for hiding this comment

tstuefe commented Feb 25, 2022

shipilev commented Feb 28, 2022

tstuefe commented Feb 28, 2022

shipilev commented Feb 28, 2022

openjdk bot commented Feb 28, 2022

openjdk bot commented Feb 28, 2022

shipilev commented Feb 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 21, 2022 •

edited

Loading

shipilev commented Feb 23, 2022 •

edited

Loading

openjdk bot commented Feb 23, 2022 •

edited

Loading