RFR: 8255746: Make PrintCompilation available on a per method level

[Joshua Cao]

On Wed, 12 Oct 2022 17:15:56 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Okay, I don't have a strong opinion about it. In the old code, we somehow did it in-between: We printed UL, then with `PrintCompilation`, then started the timer and then printed to `CompilationLog::log()`. So, I'm not sure what the original intention was. Maybe someone else can comment on that, too.
>
> `elapsedTimer time;` just declares variable. `TraceTime` uses it (with start/stop) later to accumulate time.
> So declaration can be placed anywhere before JVMCI code below.
> 
> But I think you should be more flexible here. Consider the case when you hit `assert` at the line 2120. Before this change you will see `PrintCompilation` line, with change - you do not. Of cause in hs_err file you will see it because you did not move `log_compile`.  I understand that you need to check directive. I would suggest to keep global `PrintCompilation` code and add variable to mark that line is already printed. Then check it in you code which checks directive to avoid duplicate.

I will move `elapsedTimer` to top. No strong opinion from myself either.

> I would suggest to keep global PrintCompilation code and add variable to mark that line is already printed. Then check it in you code which checks directive to avoid duplicate.

I don't understand this. Doesn't this just print everything?

---

We can do method specific compilation before the assert. This change seems to be working:


diff --git a/src/hotspot/share/compiler/compileBroker.cpp b/src/hotspot/share/compiler/compileBroker.cpp
index 8d0a87afc17..181dd28ba45 100644
--- a/src/hotspot/share/compiler/compileBroker.cpp
+++ b/src/hotspot/share/compiler/compileBroker.cpp
@@ -2093,6 +2093,7 @@ CompilerDirectives* DirectivesStack::_bottom = NULL;
 //
 void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
   task->print_ul();
+  elapsedTimer time;

   CompilerThread* thread = CompilerThread::current();
   ResourceMark rm(thread);
@@ -2117,11 +2118,16 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
     // native.  The NoHandleMark before the transition should catch
     // any cases where this occurs in the future.
     methodHandle method(thread, task->method());
-    assert(!method->is_native(), "no longer compile natives");

     // Look up matching directives
     directive = DirectivesStack::getMatchingDirective(method, comp);
     task->set_directive(directive);
+    if (task->directive()->PrintCompilationOption) {
+      ResourceMark rm;
+      task->print_tty();
+    }
+
+    assert(!method->is_native(), "no longer compile natives");

     // Update compile information when using perfdata.
     if (UsePerfData) {
@@ -2131,12 +2137,6 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
     DTRACE_METHOD_COMPILE_BEGIN_PROBE(method, compiler_name(task_level));
   }

-  if (task->directive()->PrintCompilationOption) {
-    ResourceMark rm;
-    task->print_tty();
-  }
-  elapsedTimer time;
-
   should_break = directive->BreakAtCompileOption || task->check_break_at_flags();
   if (should_log && !directive->LogOption) {
     should_log = false;


Although there is still possible that there is failed assertion before printing, i.e. when calling `CompilerThread::current()`. I feel like its ok to just look at hs_err in these cases.

-------------

PR: https://git.openjdk.org/jdk/pull/10668