RFR(M): 8198756: Lazy allocation of compiler threads
Doerr, Martin
martin.doerr at sap.com
Tue Apr 10 12:56:27 UTC 2018
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