RFR(M): 8198756: Lazy allocation of compiler threads

[Doerr, Martin]

Hi Vladimir,

I have addressed your proposals with this new webrev:
http://cr.openjdk.java.net/~mdoerr/8198756_CompilerCount/webrev.02/

Please take a look.

Thanks for your support. Best regards,
Martin


-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Montag, 26. März 2018 23:15
To: Doerr, Martin <martin.doerr at sap.com>
Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: RFR(M): 8198756: Lazy allocation of compiler threads

Hi Martin,

We can't delete _log when deleting CompilerThread. Log is referenced 
globally and used on VM exit to generate final log file when 
-XX:+LogCompilation is specified. compilercontrol tests passed after I 
change it:

+CompilerThread::~CompilerThread() {
+  // Delete objects which were allocated on heap.
+  delete _counters;
+  // _log is referenced in global CompileLog::_first chain and used on 
exit.
+}

I also see that we C1 compiler threads are removed too soon which cause 
their re-activation again. This may eat memory:

$ java -XX:+TraceCompilerThreads -XX:+LogCompilation t
Added initial compiler thread C2 CompilerThread0
Added initial compiler thread C1 CompilerThread0
Warning: TraceDependencies results may be inflated by VerifyDependencies
Added compiler thread C1 CompilerThread1 (available memory: 37040MB)
Added compiler thread C1 CompilerThread2 (available memory: 37033MB)
Added compiler thread C1 CompilerThread3 (available memory: 37032MB)
Removing compiler thread C1 CompilerThread3
Removing compiler thread C1 CompilerThread2
Removing compiler thread C1 CompilerThread1
Added compiler thread C1 CompilerThread1 (available memory: 37027MB)

May be we should take into account for how long these threads are not used.

Thanks,
Vladimir

On 3/23/18 5:58 PM, Vladimir Kozlov wrote:
> On 3/23/18 10:37 AM, Doerr, Martin wrote:
>> Hi Vladimir,
>>
>> thanks for the quick reply. Just a few answers. I'll take a closer 
>> look next week.
>>
>>> You can't delete thread when it is NULL
>> C++ supports calling delete NULL so I think it would be uncommon to 
>> check it. If there's a problem, I think the delete operator should get 
>> fixed.
>>
>> "If expression evaluates to a null pointer value, no destructors are 
>> called, and the deallocation function may or may not be called (it's 
>> implementation-defined), but the default deallocation functions are 
>> guaranteed to do nothing when handed a null pointer." [1]
> 
> I am sure our code analyzing tool, which we use to check code 
> correctness, will compliant about it.
> 
>>
>>> We may need to free corresponding java thread object when we remove 
>>> compiler threads.
>> I think it would be bad to remove the Java Thread objects because we'd 
>> need to recreate them which is rather expensive and violates the 
>> design principle that Compiler Threads are not allowed to call Java. 
>> Removing them wouldn't save much memory. Keeping them in global 
>> handles seems to be beneficial and makes this change easier.
> 
> Okay.
> 
>>
>>> And I thought we would need to add only one threads each time when we 
>>> hit some queue size threshold. At the start queues filled up very 
>>> fast so you may end up creating all compiler threads.
>> My current formula only creates as much compiler threads so that there 
>> exist 2 compile jobs per thread. I think this is better for startup, 
>> but we can reevaluate this.
> 
> Would be nice to see graph how number of compiler threads change with 
> time depending on load for some applications (for example, jbb2005 and 
> specjvm2008 if you have them)?
> 
>>
>> Thanks for the improvement proposals. I'll implement them next week. 
>> Nevertheless, the current version can already be tested.
> 
> I started our testing.
> 
> I just remember that we may need to treat -Xcomp and CTW cases specially.
> 
> I also ran jtreg testing locally on x64 linux for compiler/jvmci tests. 
> And also tier1 compiler tests with Graal as JIT 
> (-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI 
> -XX:+TieredCompilation -XX:+UseJVMCICompiler -Djvmci.Compiler=graal).
> They passed. I think JVMCI code is fine.
> 
> But I see crash in CompileLog::finish_log_on_error() function in 
> compiler/compilercontrol jtreg tests (they are not in tier1) with normal 
> jtreg runs:
> 
> FAILED: compiler/compilercontrol/commandfile/LogTest.java
> FAILED: compiler/compilercontrol/commands/LogTest.java
> FAILED: compiler/compilercontrol/directives/LogTest.java
> FAILED: compiler/compilercontrol/jcmd/AddLogTest.java
> FAILED: compiler/compilercontrol/jcmd/StressAddMultiThreadedTest.java
> FAILED: compiler/compilercontrol/logcompilation/LogTest.java
> 
> I started performance testing too.
> 
> Thanks,
> Vladimir
> 
>>
>> Best regards,
>> Martin
>>
>>
>> [1] http://en.cppreference.com/w/cpp/language/delete
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Freitag, 23. März 2018 18:17
>> To: Doerr, Martin <martin.doerr at sap.com>
>> Cc: Igor Veresov (igor.veresov at oracle.com) <igor.veresov at oracle.com>; 
>> White, Derek <Derek.White at cavium.com>; 
>> 'hotspot-compiler-dev at openjdk.java.net' 
>> <hotspot-compiler-dev at openjdk.java.net>
>> Subject: Re: RFR(M): 8198756: Lazy allocation of compiler threads
>>
>> Very cool!
>>
>> Few thoughts.
>>
>> You can't delete thread when it is NULL (missing check or refactor code):
>>
>>      if (thread == NULL || thread->osthread() == NULL) {
>> +    if (UseDynamicNumberOfCompilerThreads && 
>> comp->num_compiler_threads() > 0) {
>> +      delete thread;
>>
>> Why not keep handle instead of returning naked oop from 
>> create_thread_oop()? You create Handle again
>>
>> Start fields names with _ to distinguish them from local variable:
>>
>> +  static int c1_count, c2_count;
>>
>> In possibly_add_compiler_threads() you can use c2_count instead of 
>> calling compile_count() again and array size is fixed
>> already:
>>
>> +    int new_c2_count = MIN3(_c2_compile_queue->size() / 2,
>> +        
>> CompilationPolicy::policy()->compiler_count(CompLevel_full_optimization),
>>
>> And I thought we would need to add only one threads each time when we 
>> hit some queue size threshold. At the start queues
>> filled up very fast so you may end up creating all compiler threads. 
>> Or we may need more complex formula.
>>
>> We may need to free corresponding java thread object when we remove 
>> compiler threads.
>>
>> Thanks,
>> Vladimir
>>
>> On 3/23/18 8:04 AM, Doerr, Martin wrote:
>>> Hi Vladimir,
>>>
>>> thanks for updating the RFE. I already had similar ideas so I've 
>>> implemented a prototype.
>>>
>>> I'll be glad if you can support this effort.
>>>
>>> My implementation starts only one thread per type (C1/C2) initially. 
>>> Compiler threads start additional threads depending
>>> on the compile queue size, the available memory and the predetermined 
>>> maximum. The Java Thread objects get created
>>> during startup so the Compiler Threads don't need to call Java.
>>>
>>> The heuristics (in possibly_add_compiler_threads()) are just an 
>>> initial proposal and we may want to add tuning
>>> parameters or different numbers.
>>>
>>> Threads get stopped in reverse order as they were created when their 
>>> compile queue is empty for some time.
>>>
>>> The feature can be switched by 
>>> -XX:+/-UseDynamicNumberOfCompilerThreads. Thread creating and removal 
>>> can be traced by
>>> -XX:+TraceCompilerThreads.
>>>
>>> Webrev is here:
>>>
>>> http://cr.openjdk.java.net/~mdoerr/8198756_CompilerCount/webrev.01/
>>>
>>> The following issues need to get addressed, yet:
>>>
>>> -Test JVMCI support. (I'm not familiar with it.)
>>>
>>> -Possible memory leaks. I've added some delete calls when a thread 
>>> dies, but they may be incomplete.
>>>
>>> -Logging.
>>>
>>> -Performance and memory consumption evaluation.
>>>
>>> It would be great to get support and advice for these issues.
>>>
>>> Best regards,
>>>
>>> Martin
>>>