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

[Albert]

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