Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[David Holmes]

Forgot to add - no need to see an updated webrev.

Thanks,
David

On 11/05/2018 4:22 PM, David Holmes wrote:

Hi Jini,

On 11/05/2018 5:34 AM, Jini George wrote:

Hi David,

Thank you very much for the review and for the clarifications offline! 
I have addressed your comments and have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html


Looks fine.

A couple of minor comments on the test:

You don't need:

   79 LingeredApp.stopApp(theApp);

as it is done in the finally block.

Can you add comments in checkForTruncation stating where the expected 
values were obtained from.


Thanks,
David


Thanks,
Jini.


On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,

On 4/05/2018 2:17 AM, Jini George wrote:

Hello!

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8195613

([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)

Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/


Actual fix seems fine.

I have a few comments on the test ...

- Why are you only testing on 64-bit? The truncation would happen on 
32-bit as well.


- CheckForTruncation seems rather complicated, not sure why we need 
to look for two different things with one being amd64 specific??


- We've had discussions in the past about splitting strings on \n or 
\r\n depending on whether it's Windows or not. Better to use 
String.split("\R") regex function.


- Exception message strings should not contain newline characters. 
Also you can simplify them:


Reading XXX: got value NNN but expected MMM

And you don't need to define a local variable "String message" you 
can just:


throw new Exception("Reading XXX: got value " + value + " but 
expected " + expected);


Thanks,
David
-


Testing: The SA tests passed on Mach5.

Thanks,
Jini.

Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[David Holmes]

Hi Jini,

On 11/05/2018 5:34 AM, Jini George wrote:

Hi David,

Thank you very much for the review and for the clarifications offline! I 
have addressed your comments and have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html


Looks fine.

A couple of minor comments on the test:

You don't need:

  79 LingeredApp.stopApp(theApp);

as it is done in the finally block.

Can you add comments in checkForTruncation stating where the expected 
values were obtained from.


Thanks,
David


Thanks,
Jini.


On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,

On 4/05/2018 2:17 AM, Jini George wrote:

Hello!

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8195613

([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)

Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/


Actual fix seems fine.

I have a few comments on the test ...

- Why are you only testing on 64-bit? The truncation would happen on 
32-bit as well.


- CheckForTruncation seems rather complicated, not sure why we need to 
look for two different things with one being amd64 specific??


- We've had discussions in the past about splitting strings on \n or 
\r\n depending on whether it's Windows or not. Better to use 
String.split("\R") regex function.


- Exception message strings should not contain newline characters. 
Also you can simplify them:


Reading XXX: got value NNN but expected MMM

And you don't need to define a local variable "String message" you can 
just:


throw new Exception("Reading XXX: got value " + value + " but expected 
" + expected);


Thanks,
David
-


Testing: The SA tests passed on Mach5.

Thanks,
Jini.

Re: RFR(S): 8185803: JdbExprTest.sh fails in JDK10-hs nightly due to "Name unknown: java.lang.Long.MAX_VALUE "

[David Holmes]

I was going to say that you also need to remove:

  29 #  @requires os.family != "windows"
  30 #  @key intermittent

:)

But something seems fishy about this bug to me anyway - comments added 
to bug report.


Cheers,
David

On 11/05/2018 12:59 PM, Chris Plummer wrote:

Hello,

I'm withdrawing this RFR. I noticed with repeated runs it was sometimes 
failing on Solaris. It looks like for the most part the test ran ok, but 
then at the end of the log you see:


--Finish execution with sending "quit" command to JDB
--Sending cmd: quit
--Quit cmd was sent
--waitForFinish: Waiting for mydojdbCmds() to finish

And it never returns from this. It looks to be the same issue as 
JDK-8171483 , but with 
a different test. Since the shell script stability issues will be 
resolved when the scripts are converted to pure java tests by 
JDK-8179318 , I think 
it's best to just let this bug be fixed at the same time as JDK-8179318 
. I was hoping to get 
the test stable and off the problemlist with this fix, but since that's 
not 100% attainable, it's not really worth pushing the fix at this time.


thanks,

Chris

On 5/10/18 5:01 PM, Chris Plummer wrote:

Hello,

Please review the following simple fix for 8185803:

https://bugs.openjdk.java.net/browse/JDK-8185803
http://cr.openjdk.java.net/~cjplummer/8185803/webrev.00/

Although this bug has been around for nearly a year, it used to only 
fail on Windows. After the push for JDK-8198426 last month, it started 
to fail on every run. The test runs jdb, stops at a breakpoint in the 
debugee, and the issues the following command:


   print java.lang.Long.MAX_VALUE

And the response back is:

com.sun.tools.example.debug.expr.ParseException: Name unknown: 
java.lang.Long.MAX_VALUE

   java.lang.Long.MAX_VALUE = null

The issue is that Long has not been initialized (it has been loaded), 
so the java.lang.Long.MAX_VALUE symbol is not valid. This became the 
case on every run after JDK-8198426 because it removed a bunch of java 
code that executed at startup, and this code must have been causing 
Long to get initialized. I'm not sure why it used to only fail on 
Windows before JDK-8198426, but it passes now on all platforms with my 
fix, which is to add a reference to java.lang.Long in the debugee so 
the Long will always be initialized.


thanks,

Chris

Re: RFR(S): 8185803: JdbExprTest.sh fails in JDK10-hs nightly due to "Name unknown: java.lang.Long.MAX_VALUE "

[Chris Plummer]

Hello,

I'm withdrawing this RFR. I noticed with repeated runs it was sometimes 
failing on Solaris. It looks like for the most part the test ran ok, but 
then at the end of the log you see:


--Finish execution with sending "quit" command to JDB
--Sending cmd: quit
--Quit cmd was sent
--waitForFinish: Waiting for mydojdbCmds() to finish

And it never returns from this. It looks to be the same issue as 
JDK-8171483 , but with 
a different test. Since the shell script stability issues will be 
resolved when the scripts are converted to pure java tests by 
JDK-8179318 , I think 
it's best to just let this bug be fixed at the same time as JDK-8179318 
. I was hoping to get 
the test stable and off the problemlist with this fix, but since that's 
not 100% attainable, it's not really worth pushing the fix at this time.


thanks,

Chris

On 5/10/18 5:01 PM, Chris Plummer wrote:

Hello,

Please review the following simple fix for 8185803:

https://bugs.openjdk.java.net/browse/JDK-8185803
http://cr.openjdk.java.net/~cjplummer/8185803/webrev.00/

Although this bug has been around for nearly a year, it used to only 
fail on Windows. After the push for JDK-8198426 last month, it started 
to fail on every run. The test runs jdb, stops at a breakpoint in the 
debugee, and the issues the following command:


   print java.lang.Long.MAX_VALUE

And the response back is:

com.sun.tools.example.debug.expr.ParseException: Name unknown: 
java.lang.Long.MAX_VALUE

   java.lang.Long.MAX_VALUE = null

The issue is that Long has not been initialized (it has been loaded), 
so the java.lang.Long.MAX_VALUE symbol is not valid. This became the 
case on every run after JDK-8198426 because it removed a bunch of java 
code that executed at startup, and this code must have been causing 
Long to get initialized. I'm not sure why it used to only fail on 
Windows before JDK-8198426, but it passes now on all platforms with my 
fix, which is to add a reference to java.lang.Long in the debugee so 
the Long will always be initialized.


thanks,

Chris

RFR(S): 8185803: JdbExprTest.sh fails in JDK10-hs nightly due to "Name unknown: java.lang.Long.MAX_VALUE "

[Chris Plummer]

Hello,

Please review the following simple fix for 8185803:

https://bugs.openjdk.java.net/browse/JDK-8185803
http://cr.openjdk.java.net/~cjplummer/8185803/webrev.00/

Although this bug has been around for nearly a year, it used to only 
fail on Windows. After the push for JDK-8198426 last month, it started 
to fail on every run. The test runs jdb, stops at a breakpoint in the 
debugee, and the issues the following command:


   print java.lang.Long.MAX_VALUE

And the response back is:

com.sun.tools.example.debug.expr.ParseException: Name unknown: 
java.lang.Long.MAX_VALUE

   java.lang.Long.MAX_VALUE = null

The issue is that Long has not been initialized (it has been loaded), so 
the java.lang.Long.MAX_VALUE symbol is not valid. This became the case 
on every run after JDK-8198426 because it removed a bunch of java code 
that executed at startup, and this code must have been causing Long to 
get initialized. I'm not sure why it used to only fail on Windows before 
JDK-8198426, but it passes now on all platforms with my fix, which is to 
add a reference to java.lang.Long in the debugee so the Long will always 
be initialized.


thanks,

Chris

Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int

[Jini George]

Hi David,

Thank you very much for the review and for the clarifications offline! I 
have addressed your comments and have a new webrev at:


http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html

Thanks,
Jini.


On 5/4/2018 12:23 PM, David Holmes wrote:

Hi Jini,

On 4/05/2018 2:17 AM, Jini George wrote:

Hello!

Requesting reviews for:

https://bugs.openjdk.java.net/browse/JDK-8195613

([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int)

Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/


Actual fix seems fine.

I have a few comments on the test ...

- Why are you only testing on 64-bit? The truncation would happen on 
32-bit as well.


- CheckForTruncation seems rather complicated, not sure why we need to 
look for two different things with one being amd64 specific??


- We've had discussions in the past about splitting strings on \n or 
\r\n depending on whether it's Windows or not. Better to use 
String.split("\R") regex function.


- Exception message strings should not contain newline characters. Also 
you can simplify them:


Reading XXX: got value NNN but expected MMM

And you don't need to define a local variable "String message" you can 
just:


throw new Exception("Reading XXX: got value " + value + " but expected " 
+ expected);


Thanks,
David
-


Testing: The SA tests passed on Mach5.

Thanks,
Jini.

Re: jcmd - executing multiple commands at the same safepoint?

[Thomas Stüfe]

On Thu, May 10, 2018 at 9:13 AM, Kirk Pepperdine
 wrote:
> The stacking at the safe point would be a huge win. Right now thread dump 
> consistency can really confuse people as the tooling will show two threads 
> owning the same lock at seemingly the same time. Reality, it’s just that 
> people didn’t realize you get a safe point for each thread therefor there is 
> motion in the system as you’re collecting the data.

I am a bit confused. What tooling are you talking about? Thread dumps
(as in jcmd Thread.info or kill -3) prints thread stacks and does
deadlock detection, both in its separate safepoints. So I can see how
information between thread stack dumps and thread deadlock detection
dumps could be inconsistent, but not in-between threads.

>
> As an aside, it’s amazing how many dev’s aren’t aware of jcmd. Just yesterday 
> after my session at Devoxx I had someone ask me about using jfr from the 
> command line, many in that session had not seen jcmd before. The feedback 
> was, wow, this is very useful and I wished I had of known about it earlier.

Yes, jcmd is quite useful. I also really like the simple design, which
is by default up- and downward compatible (so you can talk to any VM,
new, old, it should not matter). That is just good design. We - Sap -
work to extend jcmd, to help our support folks. E.g. the whole new
VM.metaspace command chain.

..Thomas

>
> — Kirk
>
>> On May 10, 2018, at 7:50 AM, Thomas Stüfe  wrote:
>>
>> Hi Kirk,
>>
>> On Thu, May 10, 2018 at 8:07 AM, Kirk Pepperdine
>>  wrote:
>>>
 On May 10, 2018, at 7:04 AM, Thomas Stüfe  wrote:

 Oh... maybe. You are right. And I see commas are not good either since
 they are used in sub options (e.g. GC.class_stats).

 Alternative ideas could be using brackets surrounding the
 command-and-options (e.g. '{''}'):

 jcmd  { GC.class_stats columns=InstBytes,KlassBytes }  { VM.metaspace
 show-loaders }
>>>
>>> are exposed braces safe in shells?

 I think technically we would not even need delimiters, since the
 commands are as keywords limiting enough. E.g. one could just specify:

 jcmd  GC.class_stats columns=InstBytes,KlassBytes VM.metaspace show-loaders
>>>
>>> I’d vote for this form. Simple..
>>
>> I think so too.
>>
>> Note how it would influence the form of jcmd command files:
>>
>> 
>> 
>> would execute two commands sequentially at different safepoints
>>
>>  
>> would execute two commands at the same safepoint
>>
>> Arguably though, if we can stack commands at the command line, the
>> jcmd file gets less important.
>>
>> I'll play around a bit and wait what others say.
>>
>> Thanks, Thomas
>>
>>>
>>> — Kirk
>>>

 and that would be for jcmd too. It is more for the benefit of the user.

 On Thu, May 10, 2018 at 7:54 AM, Kirk Pepperdine
  wrote:
> Awesome idea! Would the semicolon would be an issue for shell scripts?
>> On May 10, 2018, at 6:52 AM, Thomas Stüfe  
>> wrote:
>>
>> Hi all,
>>
>> just asking around for opinions.
>>
>> I am playing with the idea of bundling diagnostic commands to have
>> them executed at the same safepoint, in order to get results
>> consistent with each other.  E.g. a heapdump and a corresponding
>> metaspace statistic.
>>
>> Syntax wise, I think one could specify multiple commands in jcmd
>> separated by comma or semicola:
>>
>> jcmd   ;   
>>
>> What do you think, would that be a useful addition?
>>
>> Thanks, Thomas
>
>>>
>

Re: jcmd - executing multiple commands at the same safepoint?

[Kirk Pepperdine]

The stacking at the safe point would be a huge win. Right now thread dump 
consistency can really confuse people as the tooling will show two threads 
owning the same lock at seemingly the same time. Reality, it’s just that people 
didn’t realize you get a safe point for each thread therefor there is motion in 
the system as you’re collecting the data.

As an aside, it’s amazing how many dev’s aren’t aware of jcmd. Just yesterday 
after my session at Devoxx I had someone ask me about using jfr from the 
command line, many in that session had not seen jcmd before. The feedback was, 
wow, this is very useful and I wished I had of known about it earlier.

— Kirk

> On May 10, 2018, at 7:50 AM, Thomas Stüfe  wrote:
> 
> Hi Kirk,
> 
> On Thu, May 10, 2018 at 8:07 AM, Kirk Pepperdine
>  wrote:
>> 
>>> On May 10, 2018, at 7:04 AM, Thomas Stüfe  wrote:
>>> 
>>> Oh... maybe. You are right. And I see commas are not good either since
>>> they are used in sub options (e.g. GC.class_stats).
>>> 
>>> Alternative ideas could be using brackets surrounding the
>>> command-and-options (e.g. '{''}'):
>>> 
>>> jcmd  { GC.class_stats columns=InstBytes,KlassBytes }  { VM.metaspace
>>> show-loaders }
>> 
>> are exposed braces safe in shells?
>>> 
>>> I think technically we would not even need delimiters, since the
>>> commands are as keywords limiting enough. E.g. one could just specify:
>>> 
>>> jcmd  GC.class_stats columns=InstBytes,KlassBytes VM.metaspace show-loaders
>> 
>> I’d vote for this form. Simple..
> 
> I think so too.
> 
> Note how it would influence the form of jcmd command files:
> 
> 
> 
> would execute two commands sequentially at different safepoints
> 
>  
> would execute two commands at the same safepoint
> 
> Arguably though, if we can stack commands at the command line, the
> jcmd file gets less important.
> 
> I'll play around a bit and wait what others say.
> 
> Thanks, Thomas
> 
>> 
>> — Kirk
>> 
>>> 
>>> and that would be for jcmd too. It is more for the benefit of the user.
>>> 
>>> On Thu, May 10, 2018 at 7:54 AM, Kirk Pepperdine
>>>  wrote:
 Awesome idea! Would the semicolon would be an issue for shell scripts?
> On May 10, 2018, at 6:52 AM, Thomas Stüfe  wrote:
> 
> Hi all,
> 
> just asking around for opinions.
> 
> I am playing with the idea of bundling diagnostic commands to have
> them executed at the same safepoint, in order to get results
> consistent with each other.  E.g. a heapdump and a corresponding
> metaspace statistic.
> 
> Syntax wise, I think one could specify multiple commands in jcmd
> separated by comma or semicola:
> 
> jcmd   ;   
> 
> What do you think, would that be a useful addition?
> 
> Thanks, Thomas
 
>>