RFR(M): 8198756: Lazy allocation of compiler threads
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Apr 5 17:12:46 UTC 2018
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