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

[Albert]

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
>>>>>>