[9] RFR(S): JDK-8040798: compiler/startup/SmallCodeCacheStartup.java timed out in RT_Baseline

[Vladimir Kozlov]

Missed change "C1 CompileQueue" (no need for re-review):

!     _c1_compile_queue  = new CompileQueue("C1 MethodQueue", 
MethodCompileQueue_lock);

Otherwise good.

Thanks,
Vladimir

On 4/27/14 10:36 PM, Albert wrote:
> Hi,
>
> could I get a second review?
>
> Best,
> Albert
>
> On 04/25/2014 07:44 AM, Albert wrote:
>> Thank you, Igor.
>>
>> Best,
>> Albert
>> On 04/24/2014 10:07 PM, Igor Veresov wrote:
>>> This looks good to me.
>>>
>>> igor
>>>
>>> On Apr 24, 2014, at 3:50 AM, Albert <albert.noll at oracle.com> wrote:
>>>
>>>> Hi Igor,
>>>>
>>>> you are right. I implemented a new version based on your suggestions.
>>>> Here is the new webrev:
>>>> http://cr.openjdk.java.net/~anoll/8040798/webrev.02/
>>>> Best,
>>>> Albert
>>>>
>>>>
>>>> On 04/23/2014 09:49 PM, Igor Veresov wrote:
>>>>> Albert,
>>>>>
>>>>> You’re doing notify_all() on the *compile queue* lock, but the
>>>>> waiting happens on one of *task* locks (there is a separate lock
>>>>> for each task). I think you have to call notify() for each
>>>>> individual task lock as you are removing the tasks for the queue.
>>>>>
>>>>> Also, you definitely don’t want to do an actual delete of the task
>>>>> (like in delete_all()). You can’t delete the task, because later in
>>>>> wait_for_completion() you check things like is_complete() and do
>>>>> free_task(), and you’re actually using the memory that has already
>>>>> been freed. So, perhaps you should implement something like
>>>>> free_all() instead that would remove the elements from queue and
>>>>> call free_task for each of them to put them back on the free list
>>>>> (and call notify() for each of them to wake the waiters). I think
>>>>> it’s ok if you just leave the tasks on the free list after that.
>>>>>
>>>>> You should however take care not to call free_task() multiple times
>>>>> (one time in free(former delete)_all() the other time in
>>>>> wait_for_completion()). Perhaps you need another flag to check for
>>>>> that, or you can just assume that if it’s a part of the compile
>>>>> shutdown the free_task() in wait_for_completion() can be skipped.
>>>>> You would need to check for this flag in the loop in
>>>>> wait_for_completion() as well. So in addition to check for
>>>>> is_completed() you would check for is_freed() (which you’ll have to
>>>>> add) or is_compilation_disabled_forever() like you did before.
>>>>>
>>>>> igor
>>>>>
>>>>> On Apr 23, 2014, at 1:22 AM, Albert <albert.noll at oracle.com> wrote:
>>>>>
>>>>>> Igor, Vladimir, thanks for looking at this.
>>>>>>
>>>>>> @Igor: You are right. If we do the notify in delete_all(), all
>>>>>> threads that wait on the lock of a compilation task
>>>>>> will wake up. I removed the timed wait in
>>>>>> CompileBroker::wait_for_completion() since I think it is not needed
>>>>>> anymore.
>>>>>>
>>>>>> @Vladimir: Please see comments inline:
>>>>>>
>>>>>> Here is the new webrev:
>>>>>> http://cr.openjdk.java.net/~anoll/8040798/webrev.01/
>>>>>>
>>>>>>
>>>>>> On 04/22/2014 07:36 PM, Vladimir Kozlov wrote:
>>>>>>> Hi Albert,
>>>>>>>
>>>>>>> compileBroker.hpp I think you want:
>>>>>>>
>>>>>>>   NOT_PRODUCT( void print(); )
>>>>>>>
>>>>>> Done.
>>>>>>> and you need to use #ifndef PRODUCT in compileBroker.cpp.
>>>>>>>
>>>>>> Done.
>>>>>>> Next comment is not finished:
>>>>>>> +     // Delete all queued compilation tasks to make
>>>>>>>
>>>>>> Done.
>>>>>>> We have 2 queues (c1 and C2). Does it means that C1 compilation
>>>>>>> may block C2 compilation too?:
>>>>>>>
>>>>>> Yes, I think we have a single bit per Method*.
>>>>>>> + /**
>>>>>>> +  * See if this compilation is already requested.
>>>>>>> +  *
>>>>>>> +  * Implementation note: there is only a single "is in queue" bit
>>>>>>> +  * for each method.  This means that the check below is overly
>>>>>>> +  * conservative in the sense that an osr compilation in the queue
>>>>>>> +  * will block a normal compilation from entering the queue (and
>>>>>>> vice
>>>>>>> +  * versa).  This can be remedied by a full queue search to
>>>>>>> disambiguate
>>>>>>> +  * cases.  If it is deemed profitable, this may be done.
>>>>>>> +  */
>>>>>>> + bool CompileBroker::compilation_is_in_queue(methodHandle method) {
>>>>>>>
>>>>>>>
>>>>>>> Why next code in CompileBroker::compile_method() is not working?:
>>>>>>>
>>>>>> Do you mean why that code does not prevent the deadlock?
>>>>>>
>>>>>> I think that code does not cover the following scenario:
>>>>>> 1) All compiler threads wait for new tasks
>>>>>> 2) We put a blocking task in the compilation queue and wake up the
>>>>>> compiler thread
>>>>>> 3) The Java thread that issued the blocking compilation calls
>>>>>> wait() on the compilation task
>>>>>> 4) We run out of code cache and shut down compilation
>>>>>> 5) All compiler threads exit the while loop in CompileQueue::get()
>>>>>> 6) The Java thread blocks forever.
>>>>>>
>>>>>> Best,
>>>>>> Albert
>>>>>>> // If the compiler is shut off due to code cache getting full
>>>>>>>     // fail out now so blocking compiles dont hang the java thread
>>>>>>>     if (!should_compile_new_jobs()) {
>>>>>>> CompilationPolicy::policy()->delay_compilation(method());
>>>>>>>       return NULL;
>>>>>>>     }
>>>>>>>     compile_method_base(method, osr_bci, comp_level, hot_method,
>>>>>>> hot_count, comment, THREAD);
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/22/14 6:42 AM, Albert wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> could I get reviews for this small change?
>>>>>>>>
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8040798
>>>>>>>>
>>>>>>>> Problem:
>>>>>>>> The test starts the JVM with 3m code cache and 64 compiler
>>>>>>>> threads. As a
>>>>>>>> result, it is very likely
>>>>>>>> that not all compiler threads can be started due to insufficient
>>>>>>>> space
>>>>>>>> in the code cache. It is also
>>>>>>>> possible that the compiler runtime initialization of C1 and/or
>>>>>>>> C2 fails.
>>>>>>>> There is a race between a
>>>>>>>> the blocking compilations (the test is started with -Xcomp) and the
>>>>>>>> process of shutting down the
>>>>>>>> compilers. More specifically, it can happen that (1) a blocking
>>>>>>>> compilation task is put into a compilation
>>>>>>>> queue, (2) the compilers are shut down, and (3) the application
>>>>>>>> thread
>>>>>>>> waits forever until the compilation
>>>>>>>> completes (The compilation will never complete, since all compiler
>>>>>>>> threads are exited).
>>>>>>>>
>>>>>>>> Solution:
>>>>>>>> Add a timeout to the wait() that waits for a compilation to
>>>>>>>> complete and
>>>>>>>> check if compilation is shut down
>>>>>>>> forever. Furthermore, if we shut down the compiler runtimes,
>>>>>>>> make sure
>>>>>>>> that we use the interpreter to
>>>>>>>> continue execution. The changes that fix the bug are in the
>>>>>>>> following
>>>>>>>> functions:
>>>>>>>>
>>>>>>>> CompileBroker::wait_for_completion(CompileTask* task)
>>>>>>>> shutdown_compiler_runtime(AbstractCompiler* comp,
>>>>>>>> CompilerThread* thread)
>>>>>>>>
>>>>>>>>
>>>>>>>> The patch also removes the destruction of the compile queue
>>>>>>>> objects in
>>>>>>>> shutdown_compiler_runtime().
>>>>>>>> The destruction is removed, since it does not reclaim significant
>>>>>>>> portions of memory, but possible introduces
>>>>>>>> problems, since pointers to the compile queues can be used at other
>>>>>>>> locations in the code. An assert ensures
>>>>>>>> that no more tasks are put into a compilation queue once the
>>>>>>>> compilers
>>>>>>>> have been shut down.
>>>>>>>>
>>>>>>>> In addition, the patch adds two locks in:
>>>>>>>>
>>>>>>>> void CompileBroker::mark_on_stack()
>>>>>>>>
>>>>>>>>
>>>>>>>> The two locks ensure that compilation tasks are not removed from
>>>>>>>> the
>>>>>>>> compilation queue or freed by a
>>>>>>>> different (compiler) thread while the method mark_on_stack is
>>>>>>>> executed.
>>>>>>>> I am not really sure if that lock
>>>>>>>> is needed. However, the lock seems to prevent concurrent
>>>>>>>> read/writes of
>>>>>>>> the compilation queue.
>>>>>>>>
>>>>>>>> The rest of the changes are simple optional refactorings.
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>> The timeout does not occur with the proposed changes. JPRT.
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~anoll/8040798/webrev.00/
>>>>>>>>
>>>>>>>> Many thanks in advance,
>>>>>>>> Albert
>>>>>>>>
>>
>