Hi Robin,

On 9/02/2018 1:50 AM, Robin Westberg wrote:
Hi David,

On 8 Feb 2018, at 04:28, David Holmes <david.hol...@oracle.com> wrote:

Hi Robin,

Adding in hotspot-runtime-dev as all the hotspot changes belong to runtime.

Thanks, sorry about that..

I had an initial look through this.

To be honest I don't like it. We seem to have to record little bits of 
information all over the place just so they can be reported by the shutdown 
event. It seems untidy. :(

Can you rename _starting_thread to _main_thread please. The use of "starting" 
in thread.hpp/cpp is really a naming anomaly. The main thread is the thread that loads 
the JVM. And that would be consistent with set_exception_in_main_thread.

Though why do we care if the main thread exited with an unhandled exception? And if we do 
care, why do we only care when the shutdown reason is ""No remaining non-daemon Java 
threads"?

I don't like the need to add os::get_abort_exit_code. Do we really need it? 
What useful information does it convey?

Right, almost all the runtime changes are made in order to try to figure out 
what the process exit code from the launcher will eventually be. For example, 
the launcher returns 1 if the main thread exited with an unhandled exception, 
but 0 otherwise. But I actually agree that this information is probably only of 
marginal use (it could always be captured from wherever Java is launched if 
someone really wants it), so it is perhaps not worth the trouble.

Yes and that particular example of launcher behaviour is an archaic throwback to C programming - where end of "main" means end of the program - IMHO. I don't think it is of interest - and certainly not worth jumping through so many hoops to make the info available.

Discussed it a bit with Erik Gahlin, and perhaps the best option here is to 
simply remove the status code field from the event, that would simplify things 
and make the code you mention above go away.

That sounds good to me. :)

It is unfortunate that you need to add beforeHalt to deal with the event 
mechanism itself being turned off (?) by a shutdown hook. That would seem to 
potentially lose a lot of event information given hooks can run in arbitrary 
order and execute arbitrary Java code. And essentially you end up recording the 
initial reason shutdown started, though potentially it could end up terminating 
for a different reason.

In this case I think it actually conveys useful information if you are trying 
to diagnose an unexpected shutdown. It should be useful to find the initial 
request of an orderly shutdown, even if something else happens during the 
shutdown sequence like a finalizer calling exit (in which case you could 
possibly end up with two shutdown events, but both may contain interesting 
information).

Yes that is useful. But I find the need to code things the way they are, unfortunate. But given current constraints, so be it.

Thanks,
David

Best regards,
Robin

Let's see what others think ...

Thanks,
David

On 8/02/2018 1:18 AM, Robin Westberg wrote:
Hi all,
Please review the following change that adds an event-based tracing event that 
is generated when the VM shuts down. The intent is to capture shutdowns that 
occur after the VM has been properly initialized (as initialization problems 
would most likely mean that the tracing framework hasn’t been properly started 
either).
Issue: https://bugs.openjdk.java.net/browse/JDK-8041626
Webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00/
Testing: hs-tier1,hs-tier2,jdk-tier1,jdk-tier2
Best regards,
Robin

Reply via email to