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

Tue Apr 29 05:37:21 UTC 2014

Thanks for the review, Vladimir.
I'll fix the typo.

Best,
Albert

On 04/28/2014 07:57 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>>>
>>>
>>