RFR (S) 8209889: RedefineStress tests crash

[coleen.phillimore at oracle.com]

Martin, Here's your version of the fix, freshly tested.
http://cr.openjdk.java.net/~coleenp/8209889.02/webrev/index.html
Thanks,
Coleen

On 10/4/18 12:09 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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
>>
>