Request for reviews (M): 6873116: Modify reexecute implementation to use pcDesc to record the reexecute bit

[Tom Rodriguez]

On Aug 19, 2009, at 1:56 PM, changpeng fang - Sun Microsystems - Santa  
Clara United States wrote:

> On 08/19/09 13:26, Tom Rodriguez wrote:
>> This looks good.  The SA fixes look right as well.  The only  
>> suggestion I might make is to pass the PcDesc into the ScopeDesc  
>> constructor instead of passing in each of the arguments.  It's ok  
>> the way it is but the arguments are just a mirror of PcDesc.
>>
> In way, the two constructors will be reduced to one. Can I use a  
> default argument to specify
> "serialized_null" considering "avoid a .hpp-.hpp dependency" comment?

The circularity is caused by SimpleScopeDesc which seems like variant  
of limited usefulness.  I don't really understand the need for two  
ScopeDesc constructors here in the first place.  The only difference  
between them is whether they decode the objects or not and that should  
probably be lazy, which would remove any difference in them.  The  
second constructor is only used when decoding line numbers for JVMTI  
which seems like a strange place to worry about decoding the objects.   
Normal stack walking would probably prefer to skip this step too.   
Switching to lazy object decoding and having a single constructor  
taking a PcDesc seems better to me.  I think we could get rid of  
SimpleScopeDesc too.  It's only used in one place,  
nmethod::preserve_callee_argument_oops, which is only called in the  
rare instance where a GC is requested while a call site is being  
resolved.  The performance difference between using a normal ScopeDesc  
and SimpleScopeDesc would be pretty minimal.

tom



>
> // Constructor
> ScopeDesc(const nmethod* code, int decode_offset, int  
> obj_decode_offset, bool reexecute);
>
> // Calls above, giving default value of "serialized_null" to the
> // "obj_decode_offset" argument.  (We don't use a default argument to
> // avoid a .hpp-.hpp dependency.)
> ScopeDesc(const nmethod* code, int decode_offset, bool reexecute);
>
> Thanks,
>
> Changpeng
>
>
>
>> By the way, does anyone know why the objects for EA are decoded  
>> eagerly instead of being decoded lazily like all the other debug  
>> info?  That seems like unneeded overhead for most uses of ScopeDesc  
>> for stack walking.
>>
>> tom
>>
>> On Aug 19, 2009, at 10:55 AM, changpeng fang - Sun Microsystems -  
>> Santa Clara United States wrote:
>>
>>> http://cr.openjdk.java.net/~cfang/6873116/webrev.00/
>>>
>>> Fixed 6873116:  Modify reexecute implementation to use pcDesc to  
>>> record the reexecute bit
>>>
>>> The implementation in this work is mostly from Christian's work on  
>>> reexecute/mh bit changes.
>>>
>>> Problem:
>>> Current implementation of reexecute bit uses DebugInfoStreams to  
>>> record the reexecute bit by compressing the reexecute bit
>>> into bci field. This makes the code look ugly  
>>> (e.g.read_bci_and_reexecute(bool& reexecute). It also makes the  
>>> future addition
>>> of debuginfo difficult.
>>>
>>> Solution:
>>> In this work, we use pcDesc to record the reexecute bit obtained  
>>> from describe_scope, and then store the reexecute bit to
>>> scopeDesc when a scopeDesc object is created.
>>>
>>> Tests:
>>> JPRT, CompileTheWord, refworkload
>>>
>>> Thanks,
>>>
>>> Changpeng
>>>
>>
>