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.com"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: HYPERLINK > "http://cr.openjdk.java.net/%7Emgronlun/8039905/webrev01/"http://cr.op > enjdk.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 > > > >
