RFR(M): 8198756: Lazy allocation of compiler threads
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 18 18:39:15 UTC 2018
Good. I updated CSR and Release notes based on this latest changes.
Waiting David's verdict.
Thanks,
Vladimir
On 4/18/18 3:19 AM, Doerr, Martin wrote:
> 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