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

Doerr, Martin martin.doerr at sap.com
Thu Apr 5 10:09:30 UTC 2018 

Hi Vladimir,

thank you very much for fixing the assertion. I've added the fix to the new webrev:
http://cr.openjdk.java.net/~mdoerr/8198756_CompilerCount/webrev.04/

Also thank you for all your support and for running tests.

I'll wait for a 2nd review. Do we need a CSR for the new switches? A release note would probably also be appreciated.

Best regards,
Martin


-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Mittwoch, 4. April 2018 19:17
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

I was not able to test these changes because because IT took down our 
machines for updates. I started testing with my changes below and I will 
let you know results.

Meanwhile you need other reviewers to look on this implementation.

To validate JVMCI run with next flags which replace C2 with Graal:

  -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI 
-XX:+UseJVMCICompiler -Djvmci.Compiler=graal

I tried simple test and it works.

There are also hotspot jtreg JVMCI test: compiler/jvmci
I ran them and they passed (except tests which are in ProblemList.txt).

I did new build of latest jdk/hs and your webrev.03. It still has 
problem listed in bug report:

java -XX:+LogCompilation -XX:-TieredCompilation -Xcomp t

#  Internal Error (src/hotspot/share/compiler/compileBroker.cpp:1706), 
pid=363246, tid=363257
#  assert(_compiler1_logs != __null) failed: must be initialized at this 
point

The assert is too strong when only one compiler is available.

I made next changes to fix it:

diff -r 51a762273af1 src/hotspot/share/compiler/compileBroker.cpp
--- a/src/hotspot/share/compiler/compileBroker.cpp
+++ b/src/hotspot/share/compiler/compileBroker.cpp
@@ -1691,10 +1691,14 @@

    // Find Compiler number by its threadObj.
    jobject* compiler_objects = c1 ? _compiler1_objects : 
_compiler2_objects;
+  assert(compiler_objects != NULL, "must be initialized at this point");
+  CompileLog** logs = c1 ? _compiler1_logs : _compiler2_logs;
+  assert(logs != NULL, "must be initialized at this point");
+  int count = c1 ? _c1_count : _c2_count;
    oop compiler_obj = ct->threadObj();
    int compiler_number = 0;
    bool found = false;
-  for (; compiler_number < (c1 ? _c1_count : _c2_count); 
compiler_number++) {
+  for (; compiler_number < count; compiler_number++) {
      if 
(oopDesc::equals(JNIHandles::resolve_non_null(compiler_objects[compiler_number]), 
compiler_obj)) {
        found = true;
        break;
@@ -1703,10 +1707,7 @@
    assert(found, "Compiler must exist at this point");

    // Determine pointer for this threads log.
-  assert(_compiler1_logs != NULL, "must be initialized at this point");
-  assert(_compiler2_logs != NULL, "must be initialized at this point");
-  CompileLog** log_ptr = c1 ? &_compiler1_logs[compiler_number]
-                            : &_compiler2_logs[compiler_number];
+  CompileLog** log_ptr = &logs[compiler_number];

    // Return old one if it exists.
    CompileLog* log = *log_ptr;





On 4/4/18 6:16 AM, Doerr, Martin wrote:
> Hi Vladimir,
> 
> our test results look good with the rebased webrev.03, but we don't build with JVMCI. So you could still see test failures which don't show up in our environment.
> 
> Do you know if there are still things missing or known problems with webrev.03?
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Doerr, Martin
> Sent: Samstag, 31. März 2018 16:49
> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>
> Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: [CAUTION] RE: RFR(M): 8198756: Lazy allocation of compiler threads
> 
> Hi Vladimir,
> 
> I have added your changes, but switched on ReduceNumberOfCompilerThreads by default:
> http://cr.openjdk.java.net/~mdoerr/8198756_CompilerCount/webrev.03/
> 
> I have also added a fix for the stack size test. I will run testing and look at further issues on Tuesday (the next business day in Germany).
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Donnerstag, 29. März 2018 19:53
> 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
> 
> Okay. I posted webrev.02 testing failures in bug report.
> 
> Thanks,
> Vladimir K
> 
> On 3/29/18 10:15 AM, Doerr, Martin wrote:
>> Hi Vladimir,
>>
>> sorry, I had missed your proposals you have added to the bug while I was sick. webrev.02 is only based on the emails.
>> I'll think about the ideas which were written in the bug next week.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Donnerstag, 29. März 2018 18:42
>> 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,
>>
>> Thank you for update.
>>
>> Does using thread->smr_delete() solve runtime/whitebox/WBStackSize.java
>> test problem I reported in RFE?
>>
>> I see you kept _c1_compile_queue->size() / 2. I think it create too many
>> C1 threads reaching max number _c1_count very fast.
>>
>> I will start testing with 02 changes and let you know results.
>>
>> Thanks,
>> Vladimir
>>
>> On 3/28/18 9:02 AM, Doerr, Martin wrote:
>>> 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
>>>>>>

More information about the hotspot-compiler-dev mailing list