Re: RFR (S) 8075030: JvmtiEnv::GetObjectSize reports incorrect java.lang.Class instance size

[serguei.spit...@oracle.com]

Hi Aleksey,

I guess, at least, this needs a CCC request.
I'm uncomfortable to support the change without knowing the history 
behind the original implementation.
Does anyone know the reasons why current implementation was chosen in 
the first place?

Are there any tools that depend on the current behavior?

Thanks,
Serguei

On 5/31/16 00:40, Aleksey Shipilev wrote:

Hi,

Please review a tiny fix in GetObjectSize:
   https://bugs.openjdk.java.net/browse/JDK-8075030
   http://cr.openjdk.java.net/~shade/8075030/webrev.01/

I think this is a leftover from Metaspace work.

For some reason, JvmtiEnv::GetObjectSize has a special case for
java.lang.Class: it returns the Klass* size, not the java.lang.Class
instance size. This is incorrect: Klass* is indeed referenced from
java.lang.Class.metadata field, but the instance size itself does not
depend on metadata size. This confuses Instrumentation.getObjectSize users.

Testing: new test; RBT, :hotspot_compiler, :hotspot_gc,
:hotspot_runtime, :hotspot_serviceability, :hotspot_misc,
:jdk_management, :jdk_instrument, :jdk_jmx, :jdk_jdi, :svc_tools.

Thanks,
-Aleksey

Re: RFR(XS): 8158033: notify_tracing() misplaced for intended purpose

[David Holmes]

Thanks for the additional info. I guess we will see how this behaves as 
time progresses.


Reviewed.

David

On 28/05/2016 2:11 AM, Markus Gronlund wrote:

Hi David,

Thanks for taking a look, pls see below.

Thanks again
Markus

-Original Message-
From: David Holmes
Sent: den 27 maj 2016 13:52
To: Markus Gronlund; serviceability-dev@openjdk.java.net
Subject: Re: RFR(XS): 8158033: notify_tracing() misplaced for intended purpose

Hi Markus,

On 27/05/2016 7:33 PM, Markus Gronlund wrote:

Greetings,

Please review this small fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8158033

Webrev: http://cr.openjdk.java.net/~mgronlun/8158033/webrev/

Description:

The intent when putting in the notify_tracing() hook into debug.cpp
(report_java_out_of_memory()) was to intercept a state believed to be
a VM termination state, especially when OOME is thrown. Since it is
totally valid that Java code catches OOME, and this location actually
goes back to Java, this is the wrong location for this hook.

In addition, the hook should not be typed for OOME only, but generic
for any exit condition (normal / OOME / crash).
This should instead have been put into java.cpp (before_exit()) and in
VMError.cpp (report_vm_die()).


In src/share/vm/runtime/java.cpp why did you move the existing event code? What 
determined that TRACE_VM_EXIT should happen at that particular point?

[MG] The reason I put in the TRACE_VM_EXIT (and moved up the event with it) here is because I would 
like the call to happen before the task that stops the WatcherThread. This is in order to have the 
"is_error_reported()" functionality still in place when calling TRACE_VM_EXIT. As this 
will be called when the VM is an unknown / corrupted state (crashing), I would like to have the 
timeout abort mechanism in place should TRACE_VM_EXIT run into anything that does not return 
properly (hangs). As for moving the event site, if TRACE_VM_EXIT deconstructs the tracing 
framework, it makes sense to have the event generated before this point. Aside, today, the current 
event site is located "too late" to be of real value.

I also wonder what TRACE_VM_ERROR might do because in the vmError code it is 
called in a signal-handling context and so is very limited in what it can 
legitimately do without potentially messing up the error reporting.

[MG] Yes this is sensitive I agree. I have put TRACE_VM_ERROR at a place where 
it will be able to handle reentrancy into the signal handler correctly. This is 
also tested by having the TRACE_VM_EXIT logic crash. In addition, 
TRACE_VM_ERROR will be made non-reentrant on initial call.

Thanks again
Markus


Thanks,
David




Thanks

Markus

Re: RFR(XS): 8158033: notify_tracing() misplaced for intended purpose

[Erik Gahlin]

Looks good!

Not a (R)eviewer

Erik

On 2016-05-27 11:33, Markus Gronlund wrote:


Greetings,

Please review this small fix:

Bug: https://bugs.openjdk.java.net/browse/JDK-8158033

Webrev: http://cr.openjdk.java.net/~mgronlun/8158033/webrev/ 



Description:

The intent when putting in the notify_tracing() hook into debug.cpp 
(report_java_out_of_memory()) was to intercept a state believed to be 
a VM termination state, especially when OOME is thrown. Since it is 
totally valid that Java code catches OOME, and this location actually 
goes back to Java, this is the wrong location for this hook.


In addition, the hook should not be typed for OOME only, but generic 
for any exit condition (normal / OOME / crash).
This should instead have been put into java.cpp (before_exit()) and in 
VMError.cpp (report_vm_die()).


Thanks

Markus

Re: PING: RFR: JDK-8155936: Boolean value should be set 1/0 or true/false via VM.set_flag jcmd

[Yasumasa Suenaga]

Hi all,

JDK-8155936 has been reviewed.
But Gerard and I think we should add more test for this feature
whether setting a boolean flag to “true” and “false” works
as expected.

So I uploaded new webrev again.
Changes from webrev.01 (reviewed) is for SetVMFlagTest.java only.

Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8155936/webrev.02/


Thanks,

Yasumasa


On 2016/05/18 2:36, Dmitry Samersoff wrote:

Yasumasa,

Looks good for me.

-Dmitry

On 2016-05-17 17:31, Yasumasa Suenaga wrote:

Hi Dmitry,

Thank you for your comment.
I've fixed them in new webrev. Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8155936/webrev.01/


Yasumasa


On 2016/05/17 19:25, Dmitry Samersoff wrote:

Yasumasa,

1. Please use strcasecmp for true/false

2. You may save one strcmp call by replacing it to
   (*arg == '0' && *(arg+1) == 0)

-Dmitry


On 2016-05-17 13:15, Yasumasa Suenaga wrote:

PING: Could you review it?
We need a reviewer.


bug id: https://bugs.openjdk.java.net/browse/JDK-8155936
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8155936/webrev.00/



Thanks,

Yasumasa


On 2016/05/06 1:18, Gerard Ziemski wrote:

I’m including serviceability mailing list.

bug id: https://bugs.openjdk.java.net/browse/JDK-8155936v
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8155936/webrev.00/


cheers


Begin forwarded message:

From: Yasumasa Suenaga 
Subject: Re: RFR: JDK-8155936: Boolean value should be set 1/0 or
true/false via VM.set_flag jcmd
Date: May 4, 2016 at 9:34:15 AM CDT
To: "hotspot-runtime-...@openjdk.java.net"

Cc: Gerard Ziemski 

Hi all,

We still need a second Reviewer.
Could you review?


Thanks,

Yasumasa


On 2016/05/04 9:32, Yasumasa Suenaga wrote:

Hi Gerard,


Reviewed and I will sponsor it.


Thanks!


Just one question: there is no existing JDK issue covering this
yet, is there? Can you file one please if none exists yet?


I do not change in jdk repos.
My change affects jinfo, however jtreg test for jinfo passed.

I ran jtreg with two directories:

 - hotspot/test/serviceability/dcmd/vm
 - jdk/test/sun/tools/jinfo

They work fine.
(JInfoRunningProcessFlagTest is failed. But it is listed in
ProblemList.)


Thanks,

Yasumasa


On 2016/05/04 3:18, Gerard Ziemski wrote:

hi Yasumasa,

Thank you for the fix, I like it - the very first time I tried
using jcmd to set a boolean value I tried using “true”, so this
will make jcmd easier to use.

Reviewed and I will sponsor it.

Just one question: there is no existing JDK issue covering this
yet, is there? Can you file one please if none exists yet?



cheers


On May 3, 2016, at 9:22 AM, Yasumasa Suenaga 
wrote:

Hi all,

This review request relates to [1].

We can change a part of -XX option via VM.set_flag jcmd.
This jcmd requires 1 or 0 as boolean value.
However, we can set 0 or not (NOT 1).

In jinfo, we can set boolean value with 1/0 or +/-.
So I think it is useful if VM.set_flag accept boolean value in
true/false.

I uploaded a webrev for this issue.
Could you review it?

 http://cr.openjdk.java.net/~ysuenaga/JDK-8155936/webrev.00/

I cannot access JPRT.
So I need a sponsor.


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019192.html

Re: RFR: 8158150: LogConfiguration::describe output can get truncated

[Robbin Ehn]

Thanks David!

/Robbin

On 05/31/2016 01:53 AM, David Holmes wrote:

Looks good!

Thanks,
David

On 31/05/2016 12:52 AM, Robbin Ehn wrote:

Hi all,

The log configuration string for an output can be longer than O_BUFLEN.
The maximum size increase for each new tag set.

Webrev: http://cr.openjdk.java.net/~rehn/8158150/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8158150

Tested with internal vm tests and jcmd VM.log list

Thanks!

/Robbin

Re: RFR: 8158150: LogConfiguration::describe output can get truncated

[Robbin Ehn]

Thanks Kim!

/Robbin

On 05/31/2016 02:06 AM, Kim Barrett wrote:

On May 30, 2016, at 10:52 AM, Robbin Ehn  wrote:

Hi all,

The log configuration string for an output can be longer than O_BUFLEN.
The maximum size increase for each new tag set.

Webrev: http://cr.openjdk.java.net/~rehn/8158150/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8158150

Tested with internal vm tests and jcmd VM.log list


Looks good.

RFR (S) 8075030: JvmtiEnv::GetObjectSize reports incorrect java.lang.Class instance size

[Aleksey Shipilev]

Hi,

Please review a tiny fix in GetObjectSize:
  https://bugs.openjdk.java.net/browse/JDK-8075030
  http://cr.openjdk.java.net/~shade/8075030/webrev.01/

I think this is a leftover from Metaspace work.

For some reason, JvmtiEnv::GetObjectSize has a special case for
java.lang.Class: it returns the Klass* size, not the java.lang.Class
instance size. This is incorrect: Klass* is indeed referenced from
java.lang.Class.metadata field, but the instance size itself does not
depend on metadata size. This confuses Instrumentation.getObjectSize users.

Testing: new test; RBT, :hotspot_compiler, :hotspot_gc,
:hotspot_runtime, :hotspot_serviceability, :hotspot_misc,
:jdk_management, :jdk_instrument, :jdk_jmx, :jdk_jdi, :svc_tools.

Thanks,
-Aleksey



signature.asc
Description: OpenPGP digital signature