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

[Vladimir Kozlov]

On Wed, 12 Oct 2022 18:51:43 GMT, Joshua Cao <duke at openjdk.org> wrote:

>> `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.

May be we are looking the wrong way. Can we do `task->set_directive(directive)` when we create `task`?
Then you don't need to move PrintCompilation code - `task->directive()` will be available at the beginning of this method.

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

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