headers
Bengt Rutisson | 1 Feb 09:50
Picon
Favicon

Re: Request for review 7141200: log some interesting information in ring buffers for crashes


Tom,

Thanks for changing this! It looks much better. The code is cleaner and 
the the information in the hs_err file is more useful.

I have only looked through the GC code changes. I think it looks good.

Some comments:

The information provided by print_heap_after_gc() is very good, but it 
does not clearly communicate what type of GC it was. In particular for 
G1 I think it would be interesting to know if it was a young GC, mixed 
GC, initial mark of full GC. I'm ok with not including this information 
now, but maybe we can file a bug to include it? Or maybe we should just 
leave it for when we re-work this based on JFR...

G1 has the notion of "extended" heap information. This can be turned on 
with the flag PrintHeapAtGCExtended. This gives a lot of information for 
each region in the heap. I tried running with your patch and turning 
this flag on. Fortunately you limit the length of the events so the 
hs_err file is not as huge as it could be. But it gets quite difficult 
to parse the hs_err file since the extended information kind of messes 
the output up. I'm attaching an example hs_err file with this output.

Again, I'm ok with leaving this as it is and just filing a bug that we 
should try to turn off "extended" when we log the information for the 
event buffer. I don't expect that PrintHeapAtGCExtended will be widely 
used by customers.
(Continue reading)

Permalink | Reply | 1 comment |
headers
Christian Thalinger | 1 Feb 11:26
Picon
Favicon

Re: Request for review (S): 7141637: JSR 292: MH spread invoker crashes with NULL argument on x86_32


On Jan 31, 2012, at 8:07 PM, Volker Simonis wrote:

> Hi,
> 
> as the test in the webrev demonstrates, the MH spread invoker
> will crashes the VM if invoked with a NULL argument on x86_32
> platforms.
> 
> This is because nn 32-bit Intel platforms, the spread invoker
> uses the 'rsi' register as temporary. But because 'rsi' also
> contains the 'saved_last_sp' on x86_32 platforms, 'rsi' has to be
> temporary saved on the stack.  If this saving is done before a
> failing 'NULL' check in the spread adapter, the following
> excpetion handling routine will be confused by the extra value on
> the stack, because it expects to find the return address here.
> 
> To fix this problem, I've exchanged the usage of the temporary
> registers 'rsi' and 'rdi' in the spread adapter such that we only
> have to save the value of 'rsi' to the stack after the NULL check
> was done:
> 
> http://cr.openjdk.java.net/~simonis/SpreadNullArg/

Thanks for the detailed bug report and the fix.  I filed:

7141637: JSR 292: MH spread invoker crashes with NULL argument on x86_32

and created and webrev just for the record:
(Continue reading)

Permalink | Reply | 1 comment |
headers
Volker Simonis | 1 Feb 11:36
Picon

Re: Request for review (S): 7141637: JSR 292: MH spread invoker crashes with NULL argument on x86_32

Looks good:)

Thank you,
Volker

On Wed, Feb 1, 2012 at 11:26 AM, Christian Thalinger
<christian.thalinger@...> wrote:
>
> On Jan 31, 2012, at 8:07 PM, Volker Simonis wrote:
>
>> Hi,
>>
>> as the test in the webrev demonstrates, the MH spread invoker
>> will crashes the VM if invoked with a NULL argument on x86_32
>> platforms.
>>
>> This is because nn 32-bit Intel platforms, the spread invoker
>> uses the 'rsi' register as temporary. But because 'rsi' also
>> contains the 'saved_last_sp' on x86_32 platforms, 'rsi' has to be
>> temporary saved on the stack.  If this saving is done before a
>> failing 'NULL' check in the spread adapter, the following
>> excpetion handling routine will be confused by the extra value on
>> the stack, because it expects to find the return address here.
>>
>> To fix this problem, I've exchanged the usage of the temporary
>> registers 'rsi' and 'rdi' in the spread adapter such that we only
>> have to save the value of 'rsi' to the stack after the NULL check
>> was done:
>>
>> http://cr.openjdk.java.net/~simonis/SpreadNullArg/
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tom Rodriguez | 1 Feb 16:16
Picon
Favicon

Re: Request for review 7141200: log some interesting information in ring buffers for crashes


On Feb 1, 2012, at 12:50 AM, Bengt Rutisson wrote:

> 
> Tom,
> 
> Thanks for changing this! It looks much better. The code is cleaner and the the information in the hs_err
file is more useful.
> 
> I have only looked through the GC code changes. I think it looks good.
> 
> Some comments:
> 
> The information provided by print_heap_after_gc() is very good, but it does not clearly communicate
what type of GC it was. In particular for G1 I think it would be interesting to know if it was a young GC, mixed
GC, initial mark of full GC. I'm ok with not including this information now, but maybe we can file a bug to
include it? Or maybe we should just leave it for when we re-work this based on JFR…

I'd like to not rework that at this point.  I'll file a bug for some enhancements.

> 
> G1 has the notion of "extended" heap information. This can be turned on with the flag
PrintHeapAtGCExtended. This gives a lot of information for each region in the heap. I tried running with
your patch and turning this flag on. Fortunately you limit the length of the events so the hs_err file is not
as huge as it could be. But it gets quite difficult to parse the hs_err file since the extended information
kind of messes the output up. I'm attaching an example hs_err file with this output.
> 
> Again, I'm ok with leaving this as it is and just filing a bug that we should try to turn off "extended" when we
log the information for the event buffer. I don't expect that PrintHeapAtGCExtended will be widely used
by customers.
(Continue reading)

Permalink | Reply | 0 comments |
headers
James Melvin | 1 Feb 20:06
Picon
Favicon

Re: RFR(S): 7123386: RFE: Preserve universal builds of HotSpot on Mac OS X

Dan,

Thanks for the excellent code review.  I've incorporated all your
suggestions.  Here is an updated webrev and JPRT job...

WEBREV:
   http://cr.openjdk.java.net/~jmelvin/7123386/webrev.01

JPRT JOB (internal):
   2012-02-01-164233.jmelvin.7123386

Thanks,

Jim

On 1/31/12 3:50 PM, Daniel D. Daugherty wrote:
> On 1/31/12 1:28 PM, James Melvin wrote:
>> Hi,
>>
>> Please review the change to HotSpot to package JVM libraries in a
>> universal binary on Mac OS X. The current plan of record is to only
>> support a 64-bit JVM on Mac OS X. This solution preserves the option to
>> additionally include other architectures in the universal binary
>> in the future. It also further encapsulates platform specific makefile
>> changes for Mac OS X builds.
>>
>> Feedback welcome...
>>
>> WEBREV:
>> http://cr.openjdk.java.net/~jmelvin/7123386/webrev.00
(Continue reading)

Permalink | Reply | 0 comments |
headers
Tom Rodriguez | 1 Feb 20:08
Picon
Favicon

Re: review for 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale

>>>> 
>>>> 
>>>> You still use tty->timestamp().milliseconds() instead of javaTimeNanos(). Not sure whether you
just didn't get to fixing it yet or if you decided against it.
>>> I decided against it.  It's a printing timestamp so matching the time stamp in tty seemed to make more
sense to me.
>> I'm about to send out a webrev for some other changes and I'm going to correct this to use javaTimeNanos to
be consistent with the other GC timestamps.  I'm also going to remove the extra braces in safepoint.cpp.
> 
> Had a quick look at the changes you sent out for 7013347. I see that you have included these fixes there. Thanks!

I think that using tty->timestamp().milliseconds() was actually correct.  GC uses
gclog_or_tty->stamp() or gclog_or_tty->date_stamp() for all the times printed in the logs that I can
see, which uses the same value as timestamp().milliseconds().  javaTimeNanos (and previous
javaTimeMillis) appears to be used for measuring elapsed times.  Am I reading this wrong?

tom

> 
> Bengt
> 
>> tom
>> 
>>>> 
>>>> 
>>>> gcLocker.hpp
>>>> 
>>>> 84   static void verify_critical_count() NOT_DEBUG_RETURN;
>>>> 
>>>> Not sure what the correct thing is here, but the GC code seems to use PRODUCT_RETURN more than
(Continue reading)

Permalink | Reply | 8 comments |
headers
Bengt Rutisson | 2 Feb 09:49
Picon
Favicon

Re: review for 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale

On 2012-02-01 20:08, Tom Rodriguez wrote:
>>>>>
>>>>> You still use tty->timestamp().milliseconds() instead of javaTimeNanos(). Not sure whether you
just didn't get to fixing it yet or if you decided against it.
>>>> I decided against it.  It's a printing timestamp so matching the time stamp in tty seemed to make more
sense to me.
>>> I'm about to send out a webrev for some other changes and I'm going to correct this to use javaTimeNanos to
be consistent with the other GC timestamps.  I'm also going to remove the extra braces in safepoint.cpp.
>> Had a quick look at the changes you sent out for 7013347. I see that you have included these fixes there. Thanks!
> I think that using tty->timestamp().milliseconds() was actually correct.  GC uses
gclog_or_tty->stamp() or gclog_or_tty->date_stamp() for all the times printed in the logs that I can
see, which uses the same value as timestamp().milliseconds().  javaTimeNanos (and previous
javaTimeMillis) appears to be used for measuring elapsed times.  Am I reading this wrong?

First, I was not really arguing for doing this one way or the other. It 
was just not clear to me from the review comments if you wanted to 
change to javaTimeNanos() or not.

You are right about the use in the GC code. We recently changed to use 
javaTimeNanos() for time intervals, but we use 
tty->timestamp().seconds() to print the time stamps. If I remember 
correctly, you were logging how long a thread had to wait, so it seemed 
like an interval to me. But, as I said, I was mostly just wondering what 
the decision was.

It would probably be a good idea to use the same time source for time 
stamps and intervals. When we were using javaTimeMillis() I think it was 
consistent, but maybe we should now change to javaTimeNanos() for 
tty->timestamp()?
(Continue reading)

Permalink | Reply | 7 comments |
headers
David Holmes | 2 Feb 10:31
Picon
Favicon

Re: review for 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale

On 2/02/2012 6:49 PM, Bengt Rutisson wrote:
> On 2012-02-01 20:08, Tom Rodriguez wrote:
>>>>>>
>>>>>> You still use tty->timestamp().milliseconds() instead of
>>>>>> javaTimeNanos(). Not sure whether you just didn't get to fixing it
>>>>>> yet or if you decided against it.
>>>>> I decided against it. It's a printing timestamp so matching the
>>>>> time stamp in tty seemed to make more sense to me.
>>>> I'm about to send out a webrev for some other changes and I'm going
>>>> to correct this to use javaTimeNanos to be consistent with the other
>>>> GC timestamps. I'm also going to remove the extra braces in
>>>> safepoint.cpp.
>>> Had a quick look at the changes you sent out for 7013347. I see that
>>> you have included these fixes there. Thanks!
>> I think that using tty->timestamp().milliseconds() was actually
>> correct. GC uses gclog_or_tty->stamp() or gclog_or_tty->date_stamp()
>> for all the times printed in the logs that I can see, which uses the
>> same value as timestamp().milliseconds(). javaTimeNanos (and previous
>> javaTimeMillis) appears to be used for measuring elapsed times. Am I
>> reading this wrong?
>
> First, I was not really arguing for doing this one way or the other. It
> was just not clear to me from the review comments if you wanted to
> change to javaTimeNanos() or not.
>
> You are right about the use in the GC code. We recently changed to use
> javaTimeNanos() for time intervals, but we use
> tty->timestamp().seconds() to print the time stamps. If I remember
> correctly, you were logging how long a thread had to wait, so it seemed
> like an interval to me. But, as I said, I was mostly just wondering what
(Continue reading)

Permalink | Reply | 6 comments |
headers
Bengt Rutisson | 2 Feb 11:46
Picon
Favicon

Re: review for 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale


2 feb 2012 kl. 10:31 skrev David Holmes <david.holmes@...>:

> On 2/02/2012 6:49 PM, Bengt Rutisson wrote:
>> On 2012-02-01 20:08, Tom Rodriguez wrote:
>>>>>>> 
>>>>>>> You still use tty->timestamp().milliseconds() instead of
>>>>>>> javaTimeNanos(). Not sure whether you just didn't get to fixing it
>>>>>>> yet or if you decided against it.
>>>>>> I decided against it. It's a printing timestamp so matching the
>>>>>> time stamp in tty seemed to make more sense to me.
>>>>> I'm about to send out a webrev for some other changes and I'm going
>>>>> to correct this to use javaTimeNanos to be consistent with the other
>>>>> GC timestamps. I'm also going to remove the extra braces in
>>>>> safepoint.cpp.
>>>> Had a quick look at the changes you sent out for 7013347. I see that
>>>> you have included these fixes there. Thanks!
>>> I think that using tty->timestamp().milliseconds() was actually
>>> correct. GC uses gclog_or_tty->stamp() or gclog_or_tty->date_stamp()
>>> for all the times printed in the logs that I can see, which uses the
>>> same value as timestamp().milliseconds(). javaTimeNanos (and previous
>>> javaTimeMillis) appears to be used for measuring elapsed times. Am I
>>> reading this wrong?
>> 
>> First, I was not really arguing for doing this one way or the other. It
>> was just not clear to me from the review comments if you wanted to
>> change to javaTimeNanos() or not.
>> 
>> You are right about the use in the GC code. We recently changed to use
>> javaTimeNanos() for time intervals, but we use
(Continue reading)

Permalink | Reply | 0 comments |
headers
Zhengyu Gu | 2 Feb 17:11
Picon
Favicon

Code review request: CR 7141259 Native stack is missing in hs_err

The cause of the problem is that decode acquires lock inside signal 
handler and generates assertion.

The decoder is only used by error handler at this point, so it is safe 
to run without lock. Locking has many undesirable consequences under 
this scenario, besides signal handler issue, the error handler can be 
invoked when the thread is holding other locks, as David Holmes suggested.

But it will also be used in upcoming native memory tracking code, which 
potentially can have multi-thread invoking decoder, so synchronization 
is needed.

The proposed fix is to let error handler set decoder to single threaded 
mode, in which mode no lock is acquired.

Please review
Webrev: http://cr.openjdk.java.net/~zgu/7141259/webrev.00/

Thanks,

-Zhengyu
Permalink | Reply | 12 comments |

Gmane