RFR(L) 8026054: New type profiling points: type of return values at calls
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Oct 10 10:11:16 PDT 2013
Looks good.
Vladimir
On 10/10/13 6:39 AM, Roland Westrelin wrote:
> Here is a new webrev:
> http://cr.openjdk.java.net/~roland/8026054/webrev.02/
>
> Roland.
>
> On Oct 10, 2013, at 1:51 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> Suggested fix for profile_return_type() seems reasonable. Add comment why we do that.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/9/13 2:37 PM, Roland Westrelin wrote:
>>> Thanks Vladimir for taking another look. See inlined.
>>>
>>> On Oct 9, 2013, at 8:44 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> On 10/9/13 9:18 AM, Roland Westrelin wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> Thanks for reviewing this.
>>>>>
>>>>>> Sorry, but I will not be able to debug this in 3 month :( TypeStackSlotEntries is already complex enough and you including return type cell into it (by using TypeEntriesAtCall) to avoid additional cell when no object return.
>>>>>>
>>>>>> Can we always have one separate cell for return value for CallTypeData and VirtualCallTypeData when arguments are profiling. I would prefer to have that cell always instead of complicated code which tries to calculate its presence. From what I understand when you profile only one argument TypeProfileArgsLimit=1 you will use additional cell for cells count even if no return will be recorded (if return type is not profiled cell count is not added in this case). It looks like the space usage will be the same as when you always generate cell for return type for this case (for 2 and more arguments you will get extra one but it is 3+1). I think it will simplify the code because you don't need to change TypeStackSlotEntries.
>>>>>>
>>>>>> So can you separate TypeStackSlotEntries and ReturnTypeEntry and don't use TypeEntriesAtCall?:
>>>>>>
>>>>>> class VirtualCallTypeData : public VirtualCallData {
>>>>>> private:
>>>>>> ! TypeStackSlotEntries _args;
>>>>>> ! ReturnTypeEntry _ret;
>>>>>>
>>>>>> Yes, CallTypeData and VirtualCallTypeData will have more additional duplicated code. You can put some common code into TypeEntries (for example, compute_cell_count() is static).
>>>>>>
>>>>>> And TypeStackSlotEntries should be ArgTypeEntries because stack slot is just data used to record arguments types.
>>>>>
>>>>> ArgTypeEntries seems to imply it's only used for arguments. In the next change, it's used for parameters. That's why I named it TypeStackSlotEntries.
>>>>
>>>> I see, then may be CallTypeEntries. Again, we are not profiling stack slots.
>>>
>>> That would sound quite confusing if I indeed keep TypeEntriesAtCall. We are not profiling stack slots but we are keeping stack slots together with the profiling. The first profiling change is in the jprt queue so if the name is changed that will happen with this second change.
>>>
>>>>
>>>>>
>>>>> What about this:
>>>>> http://cr.openjdk.java.net/~roland/8026054/webrev.01/
>>>>> ?
>>>>
>>>> It is better. About header_cell_count(), previous version was different. As I understand you always need cell_count cell to separate case when you have only 1 argument vs no profilable arguments but have ret type. Right? So my suggestion to have always ret_type cell will not help.
>>>
>>> Yes it's right. The previous version would not include a cell count if return value profiling was off and the upper bound on the number of arguments was 1. Then CallTypeData/VirtualCallTypeData would only exist for calls where we would have one and only one argument to profile. You're right that it's confusing and overkill to optimize for this rare case.
>>>
>>>>
>>>> Another question. You execute arguments profiling in assembler only if tag is CallTypeData or VirtualCallTypeData. But you don't do such check for return_type profiling. Why?
>>>
>>> Thanks for spotting that. It's a bug. I used to only have the option to turn profiling for return values on and off. If it's on and a object is returned, we profile. Otherwise we don't. But with the current change, we can decide to profile only for JSR292 and in this case, it's not sufficient to find a returned value that is an object to know whether to profile or not. The problem is that the profiling code at the call moves the mdp right after the cells to do the profiling at the invoke (we may return to the interpreter after a deopt so it was I think easier to move the mdp past the current VirtualCallTypeData/CallTypeData rather than modify the deopt code to restore the mdp at the location where the return value must be done). I think the easiest way to fix this is to add this code:
>>>
>>> void InterpreterMacroAssembler::profile_return_type(Register mdp, Register ret, Register tmp) {
>>> + assert_different_registers(mdp, ret, tmp, rsi);
>>> if (ProfileInterpreter && MethodData::profile_return()) {
>>> Label profile_continue, done;
>>>
>>> test_method_data_pointer(mdp, profile_continue);
>>>
>>> + if (MethodData::profile_return_jsr292_only()) {
>>> + Label do_profile;
>>> + cmpb(Address(rsi, 0), Bytecodes::_invokedynamic);
>>> + jcc(Assembler::equal, do_profile);
>>> + cmpb(Address(rsi, 0), Bytecodes::_invokehandle);
>>> + jcc(Assembler::equal, do_profile);
>>> + get_method(tmp);
>>> + cmpb(Address(tmp, Method::intrinsic_id_offset_in_bytes()), vmIntrinsics::_compiledLambdaForm);
>>> + jcc(Assembler::notEqual, profile_continue);
>>> +
>>> + bind(do_profile);
>>> + }
>>> +
>>> Address mdo_ret_addr(mdp, -in_bytes(ReturnTypeEntry::size()));
>>> mov(tmp, ret);
>>> profile_obj_type(tmp, mdo_ret_addr);
>>>
>>>
>>>
>>>>
>>>> About assembler. Why you need subl(tmp, 1) (there is no comment)? And why it is not done during stack_slot cell initialization?
>>>
>>> If you have a method with a single argument, it is at slot 0. Number of arguments is 1. 1 - 0 = 1 but the actual offset compared to sp is 1 - 0 - 1 = 0.
>>> That's where the 1 is coming from.
>>>
>>>>
>>>> About C1 code for ret profiling. I see only new code in c1_LIRGenerator.cpp which calls profile_arg_type() and no new assembler. Is it enough?
>>>
>>> It is. The code in c1_LIRAssembler_x86.cpp handles profiling of one object in all cases (arguments, return, parameters). LIRGenerator::profile_arg_type() generates a LIR instruction with this:
>>>
>>> __ profile_type(new LIR_Address(mdp, md_offset, T_METADATA),
>>> value.result(), exact_klass, profiled_k, new_pointer_register(), not_null, exact_signature_k != NULL);
>>>
>>> that is later emitted with the code in c1_LIRAssembler_x86.cpp
>>>
>>> I'll send a new webrev tomorrow.
>>>
>>> Roland.
>>>
>>>>
>>>> thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> I kept TypeEntriesAtCall only for the static methods but otherwise refactored as you suggested, I think.
>>>>> This also includes Chris' feedback.
>>>>>
>>>>> Roland.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 10/8/13 9:22 AM, Roland Westrelin wrote:
>>>>>>> http://cr.openjdk.java.net/~roland/8026054/webrev.00/
>>>>>>>
>>>>>>> Follow up to 8023657: New type profiling points: arguments to call
>>>>>>>
>>>>>>> 8023657 introduced TypeStackSlotEntries to keep track of types of arguments. This change adds a couple more similar classes: TypeEntriesAtCall and ReturnTypeEntry which is maybe the confusing part of the change. ReturnTypeEntry keeps track of types of return values, TypeStackSlotEntries of types of arguments. In the next type profiling change, TypeStackSlotEntries will be used for parameter type profiling. TypeEntriesAtCall is ReturnTypeEntry + TypeStackSlotEntries to do the profiling at invokes. The number of cells needed at invokes is kept in TypeEntriesAtCall (to count cells for arguments and return value) and some methods in TypeStackSlotEntries were moved to TypeEntriesAtCall as a consequence.
>>>>>>>
>>>>>>> The output of PrintMethodData is:
>>>>>>>
>>>>>>> 17 invokevirtual 3 <TestProfiling.m2(LTestProfiling$C;Ljava/lang/Object;Ljava/lang/Object;)LTestProfiling$C;>
>>>>>>> 16 bci: 17 VirtualCallData count(0) entries(1)
>>>>>>> 'TestProfiling'(4744 1.00)
>>>>>>> argument types 0: stack(1) 'TestProfiling$C'
>>>>>>> 1: stack(2) 'TestProfiling$B'
>>>>>>> return type 'TestProfiling$C'
>>>>>>>
>>>>>>> Roland.
>>>>>>>
>>>>>
>>>
>
More information about the hotspot-compiler-dev
mailing list