RFR(S): 8233193: Incorrect bailout from possibly_add_compiler_threads

[David Holmes]

On 18/11/2019 8:42 pm, Doerr, Martin wrote:
> Hi David,
> 
> thanks for the review.
> 
> 
>> That's fine but you also replaced TRAPS with "Thread* THREAD" which is
>> pointless given:
>>
>> #define TRAPS  Thread* THREAD
> 
> I know that it's technically pointless. But I think it's confusing to use TRAPS if we don't expect any exceptions.
> We just want to pass a Thread pointer.

Yes you are right. I just re-read the comments in exceptions.hpp :)

Thanks,
David

> 
>> May I suggest a comment then:
>>
>>    844 void CompileBroker::init_compiler_sweeper_threads() {
>>          // Ensure any exceptions lead to vm_exit_during_initialization
>>    845   EXCEPTION_MARK;
> 
> Added. I also added:
> 
> @@ -647,6 +647,7 @@
>     // totalTime performance counter is always created as it is required
>     // by the implementation of java.lang.management.CompilationMBean.
>     {
> +    // Ensure OOM leads to vm_exit_during_initialization.
>       EXCEPTION_MARK;
>       _perf_total_compilation =
>                    PerfDataManager::create_counter(JAVA_CI, "totalTime",
> 
> 
> 
> Best regards,
> Martin
> 
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Montag, 18. November 2019 04:46
>> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-
>> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
>> Cc: David Holmes <David.Holmes at oracle.com>
>> Subject: Re: RFR(S): 8233193: Incorrect bailout from
>> possibly_add_compiler_threads
>>
>> Hi Martin,
>>
>> On 15/11/2019 2:18 am, Doerr, Martin wrote:
>>> Hi,
>>>
>>> I'd like to cleanup exception handling in CompileBroker a little bit.
>>>
>>> Here's my proposal:
>>>
>>> - Use THREAD instead of CHECK where no exceptions get thrown.
>>
>> That's fine but you also replaced TRAPS with "Thread* THREAD" which is
>> pointless given:
>>
>> #define TRAPS  Thread* THREAD
>>
>>> - Remove unused preload_classes.
>>
>> Ok
>>
>>> - make_thread: Rename thread to new_thread to avoid confusion with
>>> THREAD (the current compiler thread).
>>
>> Ok
>>
>>> - possibly_add_compiler_threads: Remove usage of EXCEPTION_MARK +
>> CHECK
>>> because this functions is not supposed to kill the VM on exceptions. Add
>>> assertion to caller.
>>
>> Ok
>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~mdoerr/8233193_CompileBroker/webrev.00/
>>>
>>> @David:
>>>
>>> You didn't like usage of the CHECK macro in the initialization
>>> functions, but I think they are ok.
>>>
>>> Not very nice to read, but the behavior looks ok to me.
>>>
>>> At least, I didn't find a better replacement for them. Maybe you have a
>>> proposal?
>>
>> May I suggest a comment then:
>>
>>    844 void CompileBroker::init_compiler_sweeper_threads() {
>>          // Ensure any exceptions lead to vm_exit_during_initialization
>>    845   EXCEPTION_MARK;
>>
>> Thanks,
>> David
>> -----
>>
>>> Best regards,
>>>
>>> Martin
>>>