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

[Igor Veresov]

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