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

[Vladimir Kozlov]

Tom,

I talked with Changpeng about this.
I think lazy objects decoding will needs more work and testing.

Can Changpeng push his changes as it is and I will file new
RFI for lazy decoding and fix it? After that we will be able
reduce ScopeDesc constructors.

Thanks,
Vladimir

Tom Rodriguez wrote:
> 
> 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
>>>>
>>>
>>
>