Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

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

Markus,

I've done another pass through the webrev.
It looks good in general.
Thank you for fixing it!

As for the ::locals() and ::expressions() refactoring, my guess is that 
your motivation was

to make it closer to what the GC is doing. Is it true?
You can keep it as you like because my comment is not a big or strong 
statement.

It is just my personal preference.
But, the refactoring made it harder to review the changes as the 
original mapping has gone.


As we privately discussed, this code may need further improvements 
related to the dead locals

and so, we may need to open new bugs for recognized issues.
Also, It seems you are right on the meaning of the T_CONFLICT that it is 
set for the dead locals.
However, in my bug/testcase context the T_CONFLICT value was not set for 
dead local.
The observed value of the dead local was T_INT (as uninitialized data) 
which looks pretty strange.

It is possible, we have a deal with another bug here.

Thanks,
Serguei


On 7/2/14 10:04 PM, serguei.spit...@oracle.com wrote:

Hi Markus,

Sorry for the latency.
I hope this review is still needed.

src/share/vm/interpreter/oopMapCache.hpp

A minor comment:
 The function blocks indentation was originally aligned, but this fix 
partially broke it:

-  uintptr_t entry_at(int offset){ int i = offset * bits_per_entry; 
return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
+  uintptr_t entry_at(int offset) const  { int i = offset * bits_per_entry; 
return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
  
void set_expression_stack_size(int sz){ _expression_stack_size = sz; }
  
  #ifdef ENABLE_ZAP_DEAD_LOCALS

-  bool is_dead(int offset)   { return (entry_at(offset)  (1 
 dead_bit_number)) != 0; }
+  bool is_dead(int offset) const{ return (entry_at(offset)  (1 
 dead_bit_number)) != 0; }
  #endif

  
// Lookup

-  bool match(methodHandle method, int bci)   { return _method == method() 
 _bci == bci; }
-  bool is_empty();
+  bool match(methodHandle method, int bci) const { return _method == method() 
 _bci == bci; }
+  bool is_empty() const;
||
src/share/vm/runtime/vframe.cpp

  The implementation of the ::stack_data () combined the logics from 
::locals() and ::expressions().
  As a result, the function became unreasonably more complex to have a 
deal with this combination.
  The original approach looks better even though some fragments are 
repeated twice.
  In other words, more simple and flat lines is better than less lines 
with more complexity

  as it adds another dimension to track down.


I need a little bit more time to better understand the vframe.cpp part 
of the fix.


Thanks,
Serguei





On 6/25/14 4:58 AM, Markus Grönlund wrote:


Greetings,

Kindly looking for reviews for the following change:

Bug: http://bugs.openjdk.java.net/browse/JDK-8039905

Webrev: http://cr.openjdk.java.net/~mgronlun/8039905/webrev01/ 
http://cr.openjdk.java.net/%7Emgronlun/8039905/webrev01/


Description:

JVMTI inspection code for following references makes use of a 
VM_HeapWalkOperation in order to follow-up root sets.


In bug:

https://bugs.openjdk.java.net/browse/JDK-8038624 - 
interpretedVFrame::expressions() must respect InterpreterOopMap for 
liveness, it was found that the interpretedVFrame code had a 
discrepancy between basing length information from both asking the 
interpreter frame (which saw live expression slots for calls 
instructions) as well as the oop map (which did not).


The liveness decisions for a particular BCI should be based on what 
the oopmap gives, and that was done as of that bug (8038624).


In that process, I added an assert in an attempt to validate certain 
assumptions, and this assert has triggered during nightly testing in 
some cases.


I have therefore inspected the GC code which follow-up roots from an 
interpreted frame (please see frame::oops_interpreted_do() and 
InterpreterFrameClosure::do_offset() (in frame.cpp) for reference), 
and reworked  InterpretedVFrame so that inspections for the locals 
and expression slots are done in the same way as the GC code 
(especially in regards to taking decisions on the InterpreterOopMap).


I needed to use InterpreterOopMap from a const context, and this is 
why I have constified this class where needed, as well as making 
number_of_entries() a const public accessor (to easily reach 
oop_mask length info).


Testing completed:

nsk/jdi* tests (especially the problematic ones reported for 8039905)

compiler/6507107/HeapWalkingTest

Thanks

Markus

RE: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

[Markus Grönlund]

Hi Serguei,

 

Thanks a lot for taking a look at this.

 

Yes, the primary motivation for changing this is to have this code do the 
same as the GC in respect of InterpretedVFrame (granted, we do care about 
value slots here, where GC doesn't).

 

Sorry about making this more complex, I have tried to make some of it a bit 
easier to read based on your input, please take a look if you think this is 
easier to follow:

 

New webrev suggestion: http://cr.openjdk.java.net/~mgronlun/8039905/webrev02/

 

And I agree with all the points about T_CONFLICT and dead locals that we need 
to consider as a next step.

 

Thanks again for taking the time 

 

Many thanks

Markus

 

From: Serguei Spitsyn 
Sent: den 3 juli 2014 12:59
To: Markus Grönlund; hotspot-runtime-dev; Serviceability Dev
Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath 
fail with assert(fr().interpreter_frame_expression_stack_size() = length) 
failed: error in expression stack!

 

Markus,

I've done another pass through the webrev.
It looks good in general.
Thank you for fixing it!

As for the ::locals() and ::expressions() refactoring, my guess is that your 
motivation was
to make it closer to what the GC is doing. Is it true?
You can keep it as you like because my comment is not a big or strong statement.
It is just my personal preference.
But, the refactoring made it harder to review the changes as the original 
mapping has gone.

As we privately discussed, this code may need further improvements related to 
the dead locals
and so, we may need to open new bugs for recognized issues.
Also, It seems you are right on the meaning of the T_CONFLICT that it is set 
for the dead locals.
However, in my bug/testcase context the T_CONFLICT value was not set for dead 
local.
The observed value of the dead local was T_INT (as uninitialized data) which 
looks pretty strange.
It is possible, we have a deal with another bug here.

Thanks,
Serguei


On 7/2/14 10:04 PM, HYPERLINK 
mailto:serguei.spit...@oracle.comserguei.spit...@oracle.com wrote:

Hi Markus,

Sorry for the latency.
I hope this review is still needed.

src/share/vm/interpreter/oopMapCache.hpp

A minor comment:
 The function blocks indentation was originally aligned, but this fix partially 
broke it:

-  uintptr_t entry_at(int offset)    { int i = offset * bits_per_entry; 
return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
+  uintptr_t entry_at(int offset) const  { int i = offset * 
bits_per_entry; return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
 
   void set_expression_stack_size(int sz)    { _expression_stack_size = sz; }
 
 #ifdef ENABLE_ZAP_DEAD_LOCALS
-  bool is_dead(int offset)   { return (entry_at(offset)  
(1  dead_bit_number)) != 0; }
+  bool is_dead(int offset) const    { return (entry_at(offset)  
(1  dead_bit_number)) != 0; }
 #endif
 
 
   // Lookup
-  bool match(methodHandle method, int bci)   { return _method == method() 
 _bci == bci; }
-  bool is_empty();
+  bool match(methodHandle method, int bci) const { return _method == method() 
 _bci == bci; }
+  bool is_empty() const;


src/share/vm/runtime/vframe.cpp

  The implementation of the ::stack_data () combined the logics from ::locals() 
and ::expressions().
  As a result, the function became unreasonably more complex to have a deal 
with this combination.
  The original approach looks better even though some fragments are repeated 
twice.
  In other words, more simple and flat lines is better than less lines with 
more complexity
  as it adds another dimension to track down.


I need a little bit more time to better understand the vframe.cpp part of the 
fix.

Thanks,
Serguei





On 6/25/14 4:58 AM, Markus Grönlund wrote:

Greetings,

 

Kindly looking for reviews for the following change:

 

Bug: http://bugs.openjdk.java.net/browse/JDK-8039905

Webrev: HYPERLINK 
http://cr.openjdk.java.net/%7Emgronlun/8039905/webrev01/http://cr.openjdk.java.net/~mgronlun/8039905/webrev01/
 

 

Description:

 

JVMTI inspection code for following references makes use of a 
VM_HeapWalkOperation in order to follow-up root sets.

 

In bug:

 

https://bugs.openjdk.java.net/browse/JDK-8038624 - 
interpretedVFrame::expressions() must respect InterpreterOopMap for liveness, 
it was found that the interpretedVFrame code had a discrepancy between basing 
length information from both asking the interpreter frame (which saw live 
expression slots for calls instructions) as well as the oop map (which did 
not).  

 

The liveness decisions for a particular BCI should be based on what the oopmap 
gives, and that was done as of that bug (8038624). 

 

In that process, I added an assert in an attempt to validate certain 
assumptions, and this assert has triggered during nightly testing in some 
cases. 

 

I have therefore inspected the GC code which follow-up roots from an 
interpreted frame (please see frame::oops_interpreted_do() and 

RE: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

[Markus Grönlund]

Hi Coleen,

Thanks for taking a look. What about this?

http://cr.openjdk.java.net/~mgronlun/8039905/webrev03/

If you are ok i will push this suggestion.

/Markus


-Original Message-
From: Coleen Phillimore 
Sent: den 3 juli 2014 19:33
To: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath 
fail with assert(fr().interpreter_frame_expression_stack_size() = length) 
failed: error in expression stack!


Hi Markus,

I generally agree with Serguei about adding a conditional to have one function 
do two different things, but in this case the less duplication 
with the size calculations overrides this concern.   I think your 
refactoring improves this.  I have two requests though. Can you make

  350 StackValueCollection* interpretedVFrame::stack_data(bool expressions /* 
false */) const {


The boolean not be a default parameter.   Another request that I 
normally make is that the bool be some sort of enum but since this isn't 
widespread, I think this would be overkill.

The second is can you make this parameter not be a non-const reference.

  289 static void stack_locals(const InterpreterOopMap oop_mask,
  290  const frame fr,
  291  int length,
  292  int max_locals,
  293  StackValueCollection result) {
also in stack_expressions().

You can't tell which parameter is being modified.   You can't tell 
anyway but passing *result in the caller looks weird.   Maybe making 
result the first parameter would also help show that that's what's being 
modified.

These are minor style comments.  I've verified the content makes sense.  
Please check in if no further discussion is needed.  Thank you for fixing this! 
 The gatekeepers will thank you too.

Thanks,
Coleen

On 7/3/14, 12:34 PM, Markus Grönlund wrote:
 Hi Serguei,

   

 Thanks a lot for taking a look at this.

   

 Yes, the primary motivation for changing this is to have this code do the 
 same as the GC in respect of InterpretedVFrame (granted, we do care about 
 value slots here, where GC doesn't).

   

 Sorry about making this more complex, I have tried to make some of it a bit 
 easier to read based on your input, please take a look if you think this is 
 easier to follow:

   

 New webrev suggestion: 
 http://cr.openjdk.java.net/~mgronlun/8039905/webrev02/

   

 And I agree with all the points about T_CONFLICT and dead locals that we need 
 to consider as a next step.

   

 Thanks again for taking the time

   

 Many thanks

 Markus

   

 From: Serguei Spitsyn
 Sent: den 3 juli 2014 12:59
 To: Markus Grönlund; hotspot-runtime-dev; Serviceability Dev
 Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath 
 fail with assert(fr().interpreter_frame_expression_stack_size() = length) 
 failed: error in expression stack!

   

 Markus,

 I've done another pass through the webrev.
 It looks good in general.
 Thank you for fixing it!

 As for the ::locals() and ::expressions() refactoring, my guess is 
 that your motivation was to make it closer to what the GC is doing. Is it 
 true?
 You can keep it as you like because my comment is not a big or strong 
 statement.
 It is just my personal preference.
 But, the refactoring made it harder to review the changes as the original 
 mapping has gone.

 As we privately discussed, this code may need further improvements 
 related to the dead locals and so, we may need to open new bugs for 
 recognized issues.
 Also, It seems you are right on the meaning of the T_CONFLICT that it is set 
 for the dead locals.
 However, in my bug/testcase context the T_CONFLICT value was not set for dead 
 local.
 The observed value of the dead local was T_INT (as uninitialized data) which 
 looks pretty strange.
 It is possible, we have a deal with another bug here.

 Thanks,
 Serguei


 On 7/2/14 10:04 PM, HYPERLINK 
 mailto:serguei.spit...@oracle.comserguei.spit...@oracle.com wrote:

 Hi Markus,

 Sorry for the latency.
 I hope this review is still needed.

 src/share/vm/interpreter/oopMapCache.hpp

 A minor comment:
   The function blocks indentation was originally aligned, but this fix 
 partially broke it:

 -  uintptr_t entry_at(int offset){ int i = offset * 
 bits_per_entry; return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
 +  uintptr_t entry_at(int offset) const  { int i = offset * 
 bits_per_entry; return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
   
 void set_expression_stack_size(int sz){ _expression_stack_size = sz; }
   
   #ifdef ENABLE_ZAP_DEAD_LOCALS
 -  bool is_dead(int offset)   { return (entry_at(offset) 
  (1  dead_bit_number)) != 0; }
 +  bool is_dead(int offset) const{ return (entry_at(offset)  
 (1  dead_bit_number)) != 0; }
   #endif
   
   
 // Lookup
 -  bool match(methodHandle method, int bci)   { return _method == 
 method()  

Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

[Coleen Phillimore]

On 7/3/14, 2:38 PM, Markus Grönlund wrote:

Hi Coleen,

Thanks for taking a look. What about this?

http://cr.openjdk.java.net/~mgronlun/8039905/webrev03/

If you are ok i will push this suggestion.


Yes, I think this looks good.   Ship it!

Thanks!
Coleen



/Markus


-Original Message-
From: Coleen Phillimore
Sent: den 3 juli 2014 19:33
To: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with 
assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in 
expression stack!


Hi Markus,

I generally agree with Serguei about adding a conditional to have one function 
do two different things, but in this case the less duplication
with the size calculations overrides this concern.   I think your
refactoring improves this.  I have two requests though. Can you make

   350 StackValueCollection* interpretedVFrame::stack_data(bool expressions /* 
false */) const {


The boolean not be a default parameter.   Another request that I
normally make is that the bool be some sort of enum but since this isn't 
widespread, I think this would be overkill.

The second is can you make this parameter not be a non-const reference.

   289 static void stack_locals(const InterpreterOopMap oop_mask,
   290  const frame fr,
   291  int length,
   292  int max_locals,
   293  StackValueCollection result) {
also in stack_expressions().

You can't tell which parameter is being modified.   You can't tell
anyway but passing *result in the caller looks weird.   Maybe making
result the first parameter would also help show that that's what's being 
modified.

These are minor style comments.  I've verified the content makes sense.
Please check in if no further discussion is needed.  Thank you for fixing this! 
 The gatekeepers will thank you too.

Thanks,
Coleen

On 7/3/14, 12:34 PM, Markus Grönlund wrote:

Hi Serguei,

   


Thanks a lot for taking a look at this.

   


Yes, the primary motivation for changing this is to have this code do the 
same as the GC in respect of InterpretedVFrame (granted, we do care about value 
slots here, where GC doesn't).

   


Sorry about making this more complex, I have tried to make some of it a bit 
easier to read based on your input, please take a look if you think this is 
easier to follow:

   


New webrev suggestion:
http://cr.openjdk.java.net/~mgronlun/8039905/webrev02/

   


And I agree with all the points about T_CONFLICT and dead locals that we need 
to consider as a next step.

   


Thanks again for taking the time

   


Many thanks

Markus

   


From: Serguei Spitsyn
Sent: den 3 juli 2014 12:59
To: Markus Grönlund; hotspot-runtime-dev; Serviceability Dev
Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with 
assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in 
expression stack!

   


Markus,

I've done another pass through the webrev.
It looks good in general.
Thank you for fixing it!

As for the ::locals() and ::expressions() refactoring, my guess is
that your motivation was to make it closer to what the GC is doing. Is it true?
You can keep it as you like because my comment is not a big or strong statement.
It is just my personal preference.
But, the refactoring made it harder to review the changes as the original 
mapping has gone.

As we privately discussed, this code may need further improvements
related to the dead locals and so, we may need to open new bugs for recognized 
issues.
Also, It seems you are right on the meaning of the T_CONFLICT that it is set 
for the dead locals.
However, in my bug/testcase context the T_CONFLICT value was not set for dead 
local.
The observed value of the dead local was T_INT (as uninitialized data) which 
looks pretty strange.
It is possible, we have a deal with another bug here.

Thanks,
Serguei


On 7/2/14 10:04 PM, HYPERLINK 
mailto:serguei.spit...@oracle.comserguei.spit...@oracle.com wrote:

Hi Markus,

Sorry for the latency.
I hope this review is still needed.

src/share/vm/interpreter/oopMapCache.hpp

A minor comment:
   The function blocks indentation was originally aligned, but this fix 
partially broke it:

-  uintptr_t entry_at(int offset){ int i = offset * bits_per_entry; 
return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
+  uintptr_t entry_at(int offset) const  { int i = offset * bits_per_entry; 
return bit_mask()[i / BitsPerWord]  (i % BitsPerWord); }
   
 void set_expression_stack_size(int sz){ _expression_stack_size = sz; }
   
   #ifdef ENABLE_ZAP_DEAD_LOCALS

-  bool is_dead(int offset)   { return (entry_at(offset)  (1 
 dead_bit_number)) != 0; }
+  bool is_dead(int offset) const{ return (entry_at(offset)  (1 
 dead_bit_number)) != 0; }
   #endif
   
   
 // Lookup

-  bool match(methodHandle method, 

RE: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

[Markus Grönlund]

Coleen, Serguei,

Thanks for your reviews!

Cheers
Markus

-Original Message-
From: Coleen Phillimore 
Sent: den 3 juli 2014 21:24
To: Markus Grönlund
Cc: hotspot-runtime-...@openjdk.java.net; Serviceability Dev
Subject: Re: RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath 
fail with assert(fr().interpreter_frame_expression_stack_size() = length) 
failed: error in expression stack!


On 7/3/14, 2:38 PM, Markus Grönlund wrote:
 Hi Coleen,

 Thanks for taking a look. What about this?

 http://cr.openjdk.java.net/~mgronlun/8039905/webrev03/

 If you are ok i will push this suggestion.

Yes, I think this looks good.   Ship it!

Thanks!
Coleen

cut

RFR(M): 8039905: heapdump/OnOOMToFile and heapdump/OnOOMToPath fail with assert(fr().interpreter_frame_expression_stack_size() = length) failed: error in expression stack!

[Markus Grönlund]

Greetings,

 

Kindly looking for reviews for the following change:

 

Bug: http://bugs.openjdk.java.net/browse/JDK-8039905

Webrev: http://cr.openjdk.java.net/~mgronlun/8039905/webrev01/ 

 

Description:

 

JVMTI inspection code for following references makes use of a 
VM_HeapWalkOperation in order to follow-up root sets.

 

In bug:

 

https://bugs.openjdk.java.net/browse/JDK-8038624 - 
interpretedVFrame::expressions() must respect InterpreterOopMap for liveness, 
it was found that the interpretedVFrame code had a discrepancy between basing 
length information from both asking the interpreter frame (which saw live 
expression slots for calls instructions) as well as the oop map (which did 
not).  

 

The liveness decisions for a particular BCI should be based on what the oopmap 
gives, and that was done as of that bug (8038624). 

 

In that process, I added an assert in an attempt to validate certain 
assumptions, and this assert has triggered during nightly testing in some 
cases. 

 

I have therefore inspected the GC code which follow-up roots from an 
interpreted frame (please see frame::oops_interpreted_do() and 
InterpreterFrameClosure::do_offset() (in frame.cpp) for reference), and 
reworked  InterpretedVFrame so that inspections for the locals and expression 
slots are done in the same way as the GC code (especially in regards to taking 
decisions on the InterpreterOopMap).

 

I needed to use InterpreterOopMap from a const context, and this is why I have 
constified this class where needed, as well as making number_of_entries() a 
const public accessor (to easily reach oop_mask length info).

 

Testing completed:

 

nsk/jdi* tests (especially the problematic ones reported for 8039905)

compiler/6507107/HeapWalkingTest

 

Thanks

Markus