[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-compiler-dev
Subject: Request for reviews (M)(resent with big modification): 6833129 : specjvm98 fails with NullPointerExc
From: Changpeng.Fang () Sun ! COM (changpeng fang - Sun Microsystems - Santa Clara United S
Date: 2009-07-30 20:10:29
Message-ID: 4A71FE35.6010004 () sun ! com
[Download RAW message or body]
On 07/30/09 11:53, Tom Rodriguez wrote:
>
> On Jul 29, 2009, at 7:04 PM, Changpeng Fang wrote:
>
>> http://cr.openjdk.java.net/~cfang/6833129/webrev.06/
>>
>> Hopefully this is the last version of update!
>>
>> It seems we clear the reexecute bit too earlier on the execution path
>> in the last update (webrev.05).
>> My original intention is not to let the reexecute state for the
>> current bytecode to leak into the
>> exception block. But I do this in the wrong place
>> (**GraphKit::make_exception_state**). Later
>> in the parser, when we "do_exception", we still need the current bci
>> and associated reexecute
>> bit before handling the catch block.
>
> I'm very leery of explicitly clearing the reexecute state. It's just
> seems likely to be buggy. It seems to me that once we set the value
> of the reexecute bit for a particular bci in a JVMState, every use of
> that state at that must respect the reexecute bit. Any use where we
> deopt must reexecute that bytecode. If an exception is actually thrown
> then the existence of the exception will override any reexecute logic,
> which is correct and fine. So given all that why do we ever need to
> explicitly clear the reexecute bit? I guess I'd like a more complete
> explanation of what will go wrong if we don't clear the reexecute bit
> in the cases where you are currently doing it.
>
> By the way, I noticed that set_bci requires that the reexecute state
> be undefined, which seems wrong to me. Shouldn't it change the state
> to undefined since we're no longer on the original bytecode?
Yes, I think reexecute bit should be changed to undefined whenever we
change the bci. In this way, we should be clear the reexecute bit
explicitly.
I was doing the reexecute bit clearing to avoid reexecute bit leaking to
other bci (then assert in set_bci). But if we do this for every new bci,
that is not needed.
Thanks,
Changpeng
> tom
>
>>
>> webrev.06 does the reexecute bit clean up in two places:
>>
>> (1) Block::set_start_map() -- reexecute bit should always be
>> initialized or reset the undefined at the block start
>> For the case of exception,
>> we will prevent the reexecute bit of the current map
>> leaking into the catch block.
>
> How exactly does it leak into catch block? Wouldn't that fix this
> problem?
>
>> (2) The place after make_runtime_call of rethrow in
>> Parse::catch_inline_exceptions where we are sure that
>> the reexecute bit is no longer needed.
>
> I'm not sure I understand why this is needed.
>
>>
>> Thanks,
>>
>> Changpeng
>>
>> On 07/24/09 10:41, changpeng fang - Sun Microsystems - Santa Clara
>> United States wrote:
>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.05/
>>>
>>> I updated the change with the addition of the assert of undefined
>>> reexecute state
>>> in set_bci(). This assert caught the problem of reexecute state
>>> leaking to the exception
>>> path. Here I propose to clear the reexecute state when the control
>>> goes to the exception
>>> path because the parser will stop parsing the current bytecode (it
>>> will either parses
>>> the catch block or goes to process exit).
>>>
>>> Thanks,
>>>
>>> Changpeng
>>>
>>>
>>>
>>>
>>> On 07/22/09 13:41, Vladimir Kozlov wrote:
>>>> I would use REState name instead of REBit.
>>>> Also why you did not add the assert we talked about in set_bci()?
>>>> Otherwise looks good.
>>>>
>>>> Vladimir
>>>>
>>>> changpeng fang - Sun Microsystems - Santa Clara United States wrote:
>>>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.04/
>>>>>
>>>>> Please take a final look at the implementation of reexecute bit.
>>>>>
>>>>> Changes from http://cr.openjdk.java.net/~cfang/6833129/webrev.03/ :
>>>>>
>>>>> (1) In C2 JVMState, use a three state field (instead of a
>>>>> boolean) to represent the reexecute bit: * class JVMState :
>>>>> public ResourceObj {*
>>>>> *+ public:*
>>>>> *+ typedef enum {*
>>>>> *+ RE_Undefined = -1, // not defined -- will be translated
>>>>> into false later*
>>>>> *+ RE_False = 0, // false -- do not reexecute*
>>>>> *+ RE_True = 1 // true -- reexecute the bytecode*
>>>>> *+ } REBit; //ReExecution Bit*
>>>>> *+ *
>>>>>
>>>>> In this way, we can keep the original defined reexecute bit (true
>>>>> or false) when we are going to determine
>>>>> the reexecute bit by bytecode.
>>>>>
>>>>> *+ if (out_jvms->should_reexecute() == JVMState::RE_Undefined &&
>>>>> //don't touch defined values*
>>>>> *+ should_reexecute_implied_by_bytecode(out_jvms)) {*
>>>>> *+ out_jvms->set_reexecute(JVMState::RE_True);*
>>>>> *+ }*
>>>>> *+ *
>>>>>
>>>>> (2) Renamed Intepreter::continuation_for to be Interpreter::
>>>>> deopt_continue_after_entry.
>>>>> Renamed Interpreter::bytecodes_to_reexecute to
>>>>> Interpreter::bytecode_should_reexecute
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Changpeng
>>>>>
>>>>>
>>>>> On 07/16/09 17:16, changpeng fang - Sun Microsystems - Santa Clara
>>>>> United States wrote:
>>>>>> http://cr.openjdk.java.net/~cfang/6833129/webrev.03/
>>>>>>
>>>>>> Problem:
>>>>>> The problem is in intrinsics Object.clone and Arrays.copyOf. When
>>>>>> de-optimization occurs on the slow path for array/instance
>>>>>> allocation, the Interpreter will continue execution in next
>>>>>> bytecode after the slow allocation calls without actual copying. The
>>>>>> fundamental problem is that the current vm does not have the
>>>>>> logic for the compilers to specify reexecution for the interpreter
>>>>>> if deoptimization happens for cases like intrinsics of
>>>>>> Object.clone and Arrays.copyOf.
>>>>>>
>>>>>> Solution:
>>>>>> We add such logic in the vm.
>>>>>> (1) For C2, we add an "reexecute" field in JVMState to specify
>>>>>> whether to reexecute this bytecode. This reexecute bit will be
>>>>>> recorded as debug information through **describe_scope. At
>>>>>> runtime, the deoptimizer will get the reexecution information
>>>>>> at the time of unpack_frame through reading the scope.
>>>>>> (2) There are some bytecodes that we have already known that the
>>>>>> interpreter should re-execute them when deoptimization
>>>>>> happens (e.g. _getfield and _putfield). For C2, we set the
>>>>>> reexecute bit for the out JVMS (youngest one) for them at the
>>>>>> time of adding safepoint edges. For C1, we simply set the
>>>>>> reexecute bit for them at the time of ** **describe_scope.
>>>>>> (3) For **Object.clone and Arrays.copyOf., we set the reexecute
>>>>>> bit and pop off the argument when we intrinsify them.
>>>>>>
>>>>>> Tests:
>>>>>> Passed specjvm98, JPRT and the test case of clone in the bug report.
>>>>>>
>>>>>>
>>>>>> Since there are several previous email exchanges, I collect the
>>>>>> questions from them and give a brief answer here:
>>>>>>
>>>>>> ============================
>>>>>> >From kvn:
>>>>>> ============================
>>>>>> >Also in src/share/vm/opto/callnode.hpp add an assert
>>>>>> >to check that bci is no changed when _restart is set
>>>>>> >void set_bci(int bci) { assert(!_restart, ""); _bci = bci; }
>>>>>>
>>>>>> I could not do this assertion here because sync_jvms() will
>>>>>> set_bci and we have already set the restart (reexecute) before
>>>>>> some sync_jvms calls. Do you think it ok for an assertion like
>>>>>> assert(_bci== bci || !_restart, ""); whih means for a new bci the
>>>>>> restart
>>>>>> should be false? Thanks.
>>>>>>
>>>>>> ===================
>>>>>> From jrose:
>>>>>> ===================
>>>>>> >Here's a nit: is_restart() in scopeDesc.hpp should be a const
>>>>>> function.
>>>>>> >I'd like to see comment in each case demonstrating why that the
>>>>>> values captured in dummy_restart, and the throwaway restart in
>>>>>> javaClasses.cpp (which should be called dummy_restart also) don't
>>>>>> matter.
>>>>>> Done! I appreciate if you can double check to see whether the
>>>>>> comments are appropriate or not. Thanks.
>>>>>>
>>>>>> >I'd still like to see the restart decision made by
>>>>>> continuation_for turned into an assert. At least compare it with
>>>>>> is_restart():
>>>>>>> pc = Interpreter::continuation_for(method(), bcp,
>>>>>>> callee_parameters, is_top_frame, use_next_mdp);
>>>>>>> + assert(is_restart() == !use_next_mdp, "");
>>>>>> >If the assert fails, there may be a bug.
>>>>>> Thanks for pointing out this. Now, after the redesign,
>>>>>> continuation_for does not make decision whether to reexecute or
>>>>>> continue. It is just used to compute the address
>>>>>> for the continuation (next bytecode). I add a new function to
>>>>>> compute the reexecution address
>>>>>> Interpreter::deopt_reexecute_entry(...). What do you think of the
>>>>>> new
>>>>>> design? Thanks.
>>>>>>
>>>>>> =========================
>>>>>> >From never:
>>>>>> =========================
>>>>>> >I think the term reexecute should be used in place of restart
>>>>>> since that terminology is used elsewhere. Actually I think
>>>>>> should_reexecute is better than is_reexecute as well.
>>>>>> Yes, reexecute better than restart here! Thanks.
>>>>>>
>>>>>> >I don't really like that the restart bit is associated with the
>>>>>> bci. It implies that any scope can be reexecuted when it fact
>>>>>> it's only the topmost one that can be. Given how these
>>>>>> describing scopes is structured I'm not sure I see a better way
>>>>>> though. I >don't see any asserts to enforce this for the
>>>>>> scopeDescs either.
>>>>>> Done ! Thanks
>>>>>> The printing forms for ScopeDesc and JVMState should indicate if
>>>>>> this is a restarted bytecode or not. The SA also needs to be
>>>>>> updated to read these modified ScopeDescs.
>>>>>> Done! Thanks.
>>>>>>
>>>>>> >I think manually toggling the restart bit back and forth should
>>>>>> be avoided. Preserve the original and pass on the modified one
>>>>>> or have a helper object that toggles the bit in it's
>>>>>> constructor/destructor.
>>>>>> I just design a new class "PreserveReexecuteAndSP" to save and
>>>>>> restore the reexecute bit and sp. Thanks.
>>>>>>
>>>>>>
>>>>>> Thank you so much for your help and inputs.
>>>>>>
>>>>>> Changpeng
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic