RFR (S) 8209889: RedefineStress tests crash

[coleen.phillimore at oracle.com]

Thank you!
Coleen

On 10/5/18 10:56 AM, Doerr, Martin wrote:
> Hi Coleen,
>
> thank you for the backport. Looks good, too.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: coleen.phillimore at oracle.com <coleen.phillimore at oracle.com>
> Sent: Freitag, 5. Oktober 2018 16:54
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev at openjdk.java.net
> Subject: Re: RFR (S) 8209889: RedefineStress tests crash
>
>
> Hi Martin, Thank you for the code review and suggestion for fix.
>
> I've created a backport issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8211775
>
> The backport doesn't merge automatically because of the removal of
> CompilerThreadHintNoPreempt (which doesn't safepoint).
>
> http://cr.openjdk.java.net/~coleenp/8211775.01/webrev/
>
> But it seems trivial.   I'm running mach5 tests on this.   Can you
> review this too?
>
> thanks,
> Coleen
>
> On 10/5/18 5:05 AM, Doerr, Martin wrote:
>> Hi Coleen,
>>
>> thanks for fixing this issue. I appreciate it. Looks good.
>>
>> Do you think it can get backported to 11u?
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of coleen.phillimore at oracle.com
>> Sent: Donnerstag, 4. Oktober 2018 19:58
>> To: hotspot-dev at openjdk.java.net
>> Subject: Re: RFR (S) 8209889: RedefineStress tests crash
>>
>>
>> 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
>>>>