Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Mandy,

> On 15 Feb 2018, at 20:10, mandy chung  wrote:
> 
> Hi Robin,
> 
> Do you want a shutdown event for every call to Runtime::exit regardless where 
> it is in the shutdown sequence?  or do you expect to get an event of the 
> first thread calling JVM_Halt?  or the first thread starting the shutdown 
> sequence but it may not be the thread halting?

I was aiming for an event from the first thread starting the shutdown sequence, 
as I think that’s probably the one you are looking for if you are diagnosing an 
unexpected shutdown. Any event generated after that point may not make it into 
the recording as the tracing framework shuts down from a shutdown hook.

If finalizers on exit go away, is it still possible that the thread starting 
the shutdown sequence isn’t the one eventually calling JVM_Halt (in the absence 
of calls to Runtime.halt)?

Best regards,
Robin

> 
> Mandy
> 
> On 2/15/18 5:35 AM, Robin Westberg wrote:
>> Hi again,
>> 
>> Here’s the (hopefully) final version:
>> 
>> Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 
>> 
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 
>> 
>> 
>> Best regards,
>> Robin
> 

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Roger,

> On 15 Feb 2018, at 17:00, Roger Riggs  wrote:
> 
> Hi Robin,
> 
> Looks fine to me.

Thanks for reviewing!

> (How is this tested?, Normal, exceptional, etc.)

The tests are part of the (currently closed) JFR tests.

Best regards,
Robin
 
> 
> Thanks, Roger
> 
> 
> On 2/15/2018 8:35 AM, Robin Westberg wrote:
>> Hi again,
>> 
>> Here’s the (hopefully) final version:
>> 
>> Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 
>> 
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 
>> 
>> 
>> Best regards,
>> Robin
>> 
>>> On 14 Feb 2018, at 16:15, Robin Westberg >> > wrote:
>>> 
>>> Hi Roger,
>>> 
 On 13 Feb 2018, at 16:17, Roger Riggs > wrote:
 
 Hi Robin,
 
 It looks like the status argument to BeforeHalt is discarded in 
 JVM_BeforeHalt
 and is not inserted into the event.
 That suggests it should be removed all the way back to Shutdown.beforeHalt.
>>> 
>>> You are right, my thinking was that the interface wouldn’t need to be 
>>> changed if we decided to revisit the event in the future and add the status 
>>> code. But that is perhaps unlikely, so can certainly remove the argument 
>>> for now.
>>> 
>>> Best regards,
>>> Robin
>>> 
 
 Roger
 
 
 
 On 2/13/2018 9:59 AM, Robin Westberg wrote:
> Hi Alan,
> 
>> On 12 Feb 2018, at 09:02, Alan Bateman > > wrote:
>> 
>> 
>> 
>> On 12/02/2018 07:07, David Holmes wrote:
 Okay, I’ve removed the code related to the status field, certainly 
 makes the change a bit less intrusive.
 
 Updated webrev: 
 http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/ 
 
 Incremental: 
 http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/ 
 
>>> This looks much cleaner/neater to me - thanks.
>>> 
>> The updates to Runtime/Shutdown seems okay.
> Thanks for reviewing!
> 
> Best regards,
> Robin
> 
>> -Alan
 
>>> 
>> 
> 

Re: RFR: 8041626: [Event Request] Shutdown reason

[David Holmes]

Looks fine to me.

Thanks,
David
-

On 15/02/2018 11:35 PM, Robin Westberg wrote:

Hi again,

Here’s the (hopefully) final version:

Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 

Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 


Best regards,
Robin


On 14 Feb 2018, at 16:15, Robin Westberg  wrote:

Hi Roger,


On 13 Feb 2018, at 16:17, Roger Riggs  wrote:

Hi Robin,

It looks like the status argument to BeforeHalt is discarded in JVM_BeforeHalt
and is not inserted into the event.
That suggests it should be removed all the way back to Shutdown.beforeHalt.


You are right, my thinking was that the interface wouldn’t need to be changed 
if we decided to revisit the event in the future and add the status code. But 
that is perhaps unlikely, so can certainly remove the argument for now.

Best regards,
Robin



Roger



On 2/13/2018 9:59 AM, Robin Westberg wrote:

Hi Alan,


On 12 Feb 2018, at 09:02, Alan Bateman  wrote:



On 12/02/2018 07:07, David Holmes wrote:

Okay, I’ve removed the code related to the status field, certainly makes the 
change a bit less intrusive.

Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/

This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin


-Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[mandy chung]

Hi Robin,

Do you want a shutdown event for every call to Runtime::exit regardless 
where it is in the shutdown sequence?  or do you expect to get an event 
of the first thread calling JVM_Halt?  or the first thread starting the 
shutdown sequence but it may not be the thread halting?


Mandy

On 2/15/18 5:35 AM, Robin Westberg wrote:

Hi again,

Here’s the (hopefully) final version:

Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 

Incremental: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 



Best regards,
Robin

Re: RFR: 8041626: [Event Request] Shutdown reason

[Roger Riggs]

Hi Robin,

Looks fine to me.

(How is this tested?, Normal, exceptional, etc.)

Thanks, Roger


On 2/15/2018 8:35 AM, Robin Westberg wrote:

Hi again,

Here’s the (hopefully) final version:

Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 

Incremental: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 



Best regards,
Robin

On 14 Feb 2018, at 16:15, Robin Westberg > wrote:


Hi Roger,

On 13 Feb 2018, at 16:17, Roger Riggs > wrote:


Hi Robin,

It looks like the status argument to BeforeHalt is discarded in 
JVM_BeforeHalt

and is not inserted into the event.
That suggests it should be removed all the way back to 
Shutdown.beforeHalt.


You are right, my thinking was that the interface wouldn’t need to be 
changed if we decided to revisit the event in the future and add the 
status code. But that is perhaps unlikely, so can certainly remove 
the argument for now.


Best regards,
Robin



Roger



On 2/13/2018 9:59 AM, Robin Westberg wrote:

Hi Alan,

On 12 Feb 2018, at 09:02, Alan Bateman > wrote:




On 12/02/2018 07:07, David Holmes wrote:
Okay, I’ve removed the code related to the status field, 
certainly makes the change a bit less intrusive.


Updated webrev: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/ 

Incremental: 
http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/ 


This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin


-Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi again,

Here’s the (hopefully) final version:

Full: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.02/ 

Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01-02/ 


Best regards,
Robin

> On 14 Feb 2018, at 16:15, Robin Westberg  wrote:
> 
> Hi Roger,
> 
>> On 13 Feb 2018, at 16:17, Roger Riggs  wrote:
>> 
>> Hi Robin,
>> 
>> It looks like the status argument to BeforeHalt is discarded in 
>> JVM_BeforeHalt
>> and is not inserted into the event.
>> That suggests it should be removed all the way back to Shutdown.beforeHalt.
> 
> You are right, my thinking was that the interface wouldn’t need to be changed 
> if we decided to revisit the event in the future and add the status code. But 
> that is perhaps unlikely, so can certainly remove the argument for now.
> 
> Best regards,
> Robin
> 
>> 
>> Roger
>> 
>> 
>> 
>> On 2/13/2018 9:59 AM, Robin Westberg wrote:
>>> Hi Alan,
>>> 
 On 12 Feb 2018, at 09:02, Alan Bateman  wrote:
 
 
 
 On 12/02/2018 07:07, David Holmes wrote:
>> Okay, I’ve removed the code related to the status field, certainly makes 
>> the change a bit less intrusive.
>> 
>> Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/
> This looks much cleaner/neater to me - thanks.
> 
 The updates to Runtime/Shutdown seems okay.
>>> Thanks for reviewing!
>>> 
>>> Best regards,
>>> Robin
>>> 
 -Alan
>> 
> 

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Roger,

> On 13 Feb 2018, at 16:17, Roger Riggs  wrote:
> 
> Hi Robin,
> 
> It looks like the status argument to BeforeHalt is discarded in JVM_BeforeHalt
> and is not inserted into the event.
> That suggests it should be removed all the way back to Shutdown.beforeHalt.

You are right, my thinking was that the interface wouldn’t need to be changed 
if we decided to revisit the event in the future and add the status code. But 
that is perhaps unlikely, so can certainly remove the argument for now.

Best regards,
Robin

> 
> Roger
> 
> 
> 
> On 2/13/2018 9:59 AM, Robin Westberg wrote:
>> Hi Alan,
>> 
>>> On 12 Feb 2018, at 09:02, Alan Bateman  wrote:
>>> 
>>> 
>>> 
>>> On 12/02/2018 07:07, David Holmes wrote:
> Okay, I’ve removed the code related to the status field, certainly makes 
> the change a bit less intrusive.
> 
> Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/
 This looks much cleaner/neater to me - thanks.
 
>>> The updates to Runtime/Shutdown seems okay.
>> Thanks for reviewing!
>> 
>> Best regards,
>> Robin
>> 
>>> -Alan
> 

Re: RFR: 8041626: [Event Request] Shutdown reason

[Roger Riggs]

Hi Robin,

It looks like the status argument to BeforeHalt is discarded in 
JVM_BeforeHalt

and is not inserted into the event.
That suggests it should be removed all the way back to Shutdown.beforeHalt.

Roger



On 2/13/2018 9:59 AM, Robin Westberg wrote:

Hi Alan,


On 12 Feb 2018, at 09:02, Alan Bateman  wrote:



On 12/02/2018 07:07, David Holmes wrote:

Okay, I’ve removed the code related to the status field, certainly makes the 
change a bit less intrusive.

Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/

This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin


-Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi David,

> On 12 Feb 2018, at 08:07, David Holmes  wrote:
> 
> Hi Robin,
> 
> On 10/02/2018 1:41 AM, Robin Westberg wrote:
>> Hi David,
>>> On 9 Feb 2018, at 05:22, David Holmes >>  >> >> wrote:
>>> 
>>> Hi Robin,
>>> 
 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.
>> Okay, I’ve removed the code related to the status field, certainly makes the 
>> change a bit less intrusive.
>> Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
>> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/
> 
> This looks much cleaner/neater to me - thanks.

Thanks for reviewing!

> Tool Nit: the git-webrev tool you use has an annoying quirk compared to our 
> own webrev tool - it first reports the number of files changed even when it 
> is reporting on an individual file! I'm used to seeing a first number that 
> represents number of lines changes - and the "1" in this case throws me as I 
> like to quickly look at the size of the change for each file when deciding 
> what order to look at them. :(

Good point, I’ll make sure the git-webrev tool is updated to more closely match 
the output of the standard webrev tool.

Best regards,
Robin

> 
> Thanks,
> David
> 
>> Best regards,
>> Robin
>>> 
>>> 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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Alan,

> On 12 Feb 2018, at 09:02, Alan Bateman  wrote:
> 
> 
> 
> On 12/02/2018 07:07, David Holmes wrote:
>> 
>>> Okay, I’ve removed the code related to the status field, certainly makes 
>>> the change a bit less intrusive.
>>> 
>>> Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
>>> Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/
>> 
>> This looks much cleaner/neater to me - thanks.
>> 
> The updates to Runtime/Shutdown seems okay.

Thanks for reviewing!

Best regards,
Robin

> 
> -Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[Alan Bateman]

On 12/02/2018 07:07, David Holmes wrote:


Okay, I’ve removed the code related to the status field, certainly 
makes the change a bit less intrusive.


Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/


This looks much cleaner/neater to me - thanks.


The updates to Runtime/Shutdown seems okay.

-Alan

Re: RFR: 8041626: [Event Request] Shutdown reason

[David Holmes]

Hi Robin,

On 10/02/2018 1:41 AM, Robin Westberg wrote:

Hi David,

On 9 Feb 2018, at 05:22, David Holmes > wrote:


Hi Robin,

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.


Okay, I’ve removed the code related to the status field, certainly makes 
the change a bit less intrusive.


Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/
Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/


This looks much cleaner/neater to me - thanks.

Tool Nit: the git-webrev tool you use has an annoying quirk compared to 
our own webrev tool - it first reports the number of files changed even 
when it is reporting on an individual file! I'm used to seeing a first 
number that represents number of lines changes - and the "1" in this 
case throws me as I like to quickly look at the size of the change for 
each file when deciding what order to look at them. :(


Thanks,
David


Best regards,
Robin



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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi David,

> On 9 Feb 2018, at 05:22, David Holmes  wrote:
> 
> Hi Robin,
> 
>> 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.

Okay, I’ve removed the code related to the status field, certainly makes the 
change a bit less intrusive.

Updated webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.01/ 

Incremental: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00-01/ 


Best regards,
Robin

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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Erik,

> On 7 Feb 2018, at 21:47, Erik Gahlin  wrote:
> 
> Hi Robin,
> 
> I can sponsor this.
> 
> I wonder if we should change the name of the event to "Shutdown" instead? It 
> will give us flexibility to add other shutdown related fields in the future.

Makes sense, I’ll rename it.

> Could you change the label and field to "Status Code" and statusCode.  Event 
> fields should have headline-style capitalization and statusCode allows us to 
> add other status related information in the future.

Removed the status field now, I’ll keep it in mind if it gets added back in the 
future.

Best regards,
Robin

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

Re: RFR: 8041626: [Event Request] Shutdown reason

[David Holmes]

Hi Robin,

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

Hi David,


On 8 Feb 2018, at 04:28, David Holmes  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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi David,

> On 8 Feb 2018, at 04:28, David Holmes  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.

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. 

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

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

Re: RFR: 8041626: [Event Request] Shutdown reason

[David Holmes]

Hi Robin,

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

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?


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.


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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi Alan,

> On 7 Feb 2018, at 16:30, Alan Bateman  wrote:
> 
> On 07/02/2018 15:18, 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
>> 
> Can you elaborate a bit on why this isn't in JVM_Halt? Is this partially to 
> help with cases where the shutdown hooks or finalizers run at exit cause 
> issues?

Sure, the main problem I had with that approach is actually that the tracing 
framework shuts down from a shutdown hook, so events generated in JVM_Halt will 
be lost. And I couldn’t think of any good way to capture the exit code and 
proper stack trace from inside that shutdown hook, but I could perhaps explore 
that option a bit further.

Best regards,
Robin

Re: RFR: 8041626: [Event Request] Shutdown reason

[Erik Gahlin]

Hi Robin,

I can sponsor this.

I wonder if we should change the name of the event to "Shutdown" 
instead? It will give us flexibility to add other shutdown related 
fields in the future.


Could you change the label and field to "Status Code" and statusCode.  
Event fields should have headline-style capitalization and statusCode 
allows us to add other status related information in the future.


Thanks
Erik


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

Re: RFR: 8041626: [Event Request] Shutdown reason

[Alan Bateman]

On 07/02/2018 15:18, 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

Can you elaborate a bit on why this isn't in JVM_Halt? Is this partially 
to help with cases where the shutdown hooks or finalizers run at exit 
cause issues?


-Alan