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

[Albert]

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