RFR(M): 8198756: Lazy allocation of compiler threads
Doerr, Martin
martin.doerr at sap.com
Wed Apr 18 10:19:04 UTC 2018
Hi Vladimir,
I agree. Here's the new webrev based on jdk/jdk (the hs version did apply without changes) and with the changed flag types:
http://cr.openjdk.java.net/~mdoerr/8198756_CompilerCount/webrev.05/
If everyone agrees, I'll update the CSR. I guess the diagnostic flag should not be part of it.
Best regards,
Martin
-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
Sent: Mittwoch, 18. April 2018 01:28
To: Doerr, Martin <martin.doerr at sap.com>
Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>; David Holmes <David.Holmes at oracle.com>
Subject: Re: RFR(M): 8198756: Lazy allocation of compiler threads
Hi Martin and David,
Sorry for later respond.
IMHO we should have one flag to switch a feature on and off - it is very easy to understand
comparing for numbering flags.
I can compromise by making ReduceNumberOfCompilerThreads flag as diagnostic. I originally propose it
to Martin because I have seen cases when compiler threads were started again after removal and
thought it could affect performance. But performance results show no difference.
InjectCompilerCreationFailure could be develop flag, I agree.
Regards,
Vladimir
On 4/10/18 5:56 AM, Doerr, Martin wrote:
> Hi Vladimir,
>
> David had proposed to use a different flag to control the new feature.
> We could use an int flag like e.g. CompilerThreadPolicy to switch between different policies (static, dynamic with/without removal, future implementations).
>
> I think this would be technically better, but UseDynamicNumberOfCompilerThreads is easier to use and similar to UseDynamicNumberOfGCThreads.
>
> He had also asked if the flag InjectCompilerCreationFailure needs to be available in product or if it should better be a develop flag.
>
> I'm fine with the current implementation, but I could live with David's proposals, too. What do you think? Would you like to discuss this internally?
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Doerr, Martin
> Sent: Freitag, 6. April 2018 12:33
> To: Vladimir Kozlov <vladimir.kozlov at oracle.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 Vladimir,
>
> Thanks for the release note sub-task. It looks good.
>
> I've created CSR JDK-8201235. Feel free to edit it if you like.
> If anything is missing from my side, please let me know.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Donnerstag, 5. April 2018 19:13
> 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
>
> Thank you, Martin
>
> On 4/5/18 3:09 AM, Doerr, Martin wrote:
>> 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/
>
> Looks good.
>
>>
>> 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.
>
> Yes, please, file CSR but only for product flags.
>
> I added Release Node sub-task. Please, review and edit the description I
> added.
>
> https://bugs.openjdk.java.net/browse/JDK-8201189
>
> Thanks,
> Vladimir
>
>>
>> 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