RFR(S): 8233193: Incorrect bailout from possibly_add_compiler_threads

[Doerr, Martin]

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.


> 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
> >