RFR (S) 8209889: RedefineStress tests crash

[coleen.phillimore at oracle.com]

On 10/4/18 10:34 AM, Doerr, Martin wrote:
>
> Hi Coleen,
>
> good catch. Looks good where you moved it.
>
> I couldn’t find a place for a NoSafepointVerifier, either.
>
> Maybe it’d be more clear if we replaced the “continue” in the NULL 
> case by an else case in the loop so the CompileTaskWrapper is the 
> first thing after the NULL check (see below).
>

I see, thanks to the magic of patch, the possibly_add_compiler_threads() 
is inside the 'else' clause where there's a compile task on the queue.  
Which makes perfect sense.  It seems like there should be a 
possibly_remove_compiler_thread() in the 'if' case but I'll leave that 
to the next person to touch this code.

Ok, I'll test this variant, which I also like.
thanks,
Coleen

> Best regards,
>
> Martin
>
> --- a/src/hotspot/share/compiler/compileBroker.cpp      Thu Oct 04 
> 14:03:13 2018 +0200
>
> +++ b/src/hotspot/share/compiler/compileBroker.cpp      Thu Oct 04 
> 16:29:45 2018 +0200
>
> @@ -1781,30 +1781,31 @@
>
>            return; // Stop this thread.
>
>          }
>
>        }
>
> -      continue;
>
> -    }
>
> -
>
> -    if (UseDynamicNumberOfCompilerThreads) {
>
> -      possibly_add_compiler_threads();
>
> -    }
>
> +    } else {
>
> +      // Assign the task to the current thread.  Mark this compilation
>
> +      // thread as active for the profiler.
>
> +      // CompileTaskWrapper also keeps the Method* from being 
> deallocated if redefinition
>
> +      // occurs after fetching the compile task off the queue.
>
> +      CompileTaskWrapper ctw(task);
>
> +      nmethodLocker result_handle;  // (handle for the nmethod 
> produced by this task)
>
> + task->set_code_handle(&result_handle);
>
> +      methodHandle method(thread, task->method());
>
> -    // Assign the task to the current thread. Mark this compilation
>
> -    // thread as active for the profiler.
>
> -    CompileTaskWrapper ctw(task);
>
> -    nmethodLocker result_handle;  // (handle for the nmethod produced 
> by this task)
>
> - task->set_code_handle(&result_handle);
>
> -    methodHandle method(thread, task->method());
>
> +      // Never compile a method if breakpoints are present in it
>
> +      if (method()->number_of_breakpoints() == 0) {
>
> +        // Compile the method.
>
> +        if ((UseCompiler || AlwaysCompileLoopMethods) && 
> CompileBroker::should_compile_new_jobs()) {
>
> +          invoke_compiler_on_method(task);
>
> +          thread->start_idle_timer();
>
> +        } else {
>
> +          // After compilation is disabled, remove remaining methods 
> from queue
>
> + method->clear_queued_for_compilation();
>
> + task->set_failure_reason("compilation is disabled");
>
> +        }
>
> +      }
>
> -    // Never compile a method if breakpoints are present in it
>
> -    if (method()->number_of_breakpoints() == 0) {
>
> -      // Compile the method.
>
> -      if ((UseCompiler || AlwaysCompileLoopMethods) && 
> CompileBroker::should_compile_new_jobs()) {
>
> -        invoke_compiler_on_method(task);
>
> -        thread->start_idle_timer();
>
> -      } else {
>
> -        // After compilation is disabled, remove remaining methods 
> from queue
>
> - method->clear_queued_for_compilation();
>
> - task->set_failure_reason("compilation is disabled");
>
> +      if (UseDynamicNumberOfCompilerThreads) {
>
> +        possibly_add_compiler_threads();
>
>        }
>
>      }
>
>    }
>
> *From:*coleen.phillimore at oracle.com <coleen.phillimore at oracle.com>
> *Sent:* Donnerstag, 4. Oktober 2018 13:45
> *To:* hotspot-dev developers <hotspot-dev at openjdk.java.net>; Doerr, 
> Martin <martin.doerr at sap.com>
> *Subject:* RFR (S) 8209889: RedefineStress tests crash
>
> Summary: Create CompileTaskWrapper before potential safepoint
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8209889.01/webrev 
> <http://cr.openjdk.java.net/%7Ecoleenp/8209889.01/webrev>
> bug link https://bugs.openjdk.java.net/browse/JDK-8209889
>
> MetadataOnStackMark needs to find the CompileTask in the current 
> thread, or else the Method isn't marked as being alive, so if it's 
> old/obsolete, it is freed.  There are checks to disable adding old 
> methods to the compile queue but if the method is redefined after this 
> point, there may be a preexisting bug that we compile an old method.  
> This change just fixes the crash.
>
> Can some compiler person (Martin?) look at where I moved 
> possibly_add_compiler_threads and let me know if that looks okay?  
> There's a comment in the change that says why you need to create the 
> CompileTaskWrapper before that call.  I couldn't find a place to put a 
> NoSafepointVerifier.
>
> Tested with the test that failed, which doesn't reproduce the bug 
> except rarely, and test/hotspot/jtreg/compiler tests. tier1 an 2 in 
> progress.
>
> Thanks,
> Coleen
>