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
