RFR(L) 8026054: New type profiling points: type of return values at calls

[Christian Thalinger]

On Oct 9, 2013, at 8:46 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Hi Chris,
> 
> Thanks for reviewing this.
> 
>> src/share/vm/c1/c1_GraphBuilder.cpp:
>> 
>> +  void profile_return_type(Value ret, ciMethod* callee, ciMethod* m = NULL, int bci = -1);
>> 
>> +void GraphBuilder::profile_return_type(Value ret, ciMethod* callee, ciMethod* m, int invoke_bci) {
>> +  assert((m == NULL) == (invoke_bci < 0), "invalid method and invalid bci together");
>> +  if (m == NULL) {
>> +    m = method();
>> +  }
>> +  if (invoke_bci < 0) {
>> +    invoke_bci = bci();
>> +  }
>> 
>> Is there potentially a problem if somehow InvocationEntryBci or InvalidOSREntryBci are passed in:
>> 
>> enum MethodCompilation {
>> InvocationEntryBci = -1,     // i.e., not a on-stack replacement compilation
>> InvalidOSREntryBci = -2
>> };
>> 
>> ?
> 
> You mean profile_return_type() being passed bci = InvocationEntryBci and overriding it? Passing InvocationEntryBci would be a bit problem here and I don't see how it can happen.

Yeah, I'm pretty sure it doesn't happen but I wanted to bring it up.  I think my point is maybe -1 is not the best default value here.

> 
>> +         int bci = invoke_bci;
>> 
>> Why are you having a new variable here?
> 
> There's another change coming on top of this one. I broke a big change in pieces to the best I could but some code may still look a bit strange. Thanks for spotting it. I'll get rid of bci.

Got it.

> 
>> 
>> +           ciTypeEntriesAtCall* args_and_ret = data->is_CallTypeData() ? ((ciCallTypeData*)data)->args_and_ret() : ((ciVirtualCallTypeData*)data)->args_and_ret();
>> 
>> !   const TypeEntriesAtCall* args_and_ret() const { return &_args_and_ret; }
>> 
>> I should've mentioned this in the previous review but can't we make args_and_ret() a virtual method?
> 
> In what class? ProfileData?

I don't know; I've lost track of the class hierarchy.  If the class hierarchy is sane there must be a super class that's suitable.

> Wouldn't that expose some implementation details at a level where they don't make much sense. Also on ci* side, there's no common root class and we probably don't want ci* stuff to leak in methodData.hpp.

Why isn't there a common super class in ci?

> 
>> src/share/vm/oops/methodData.cpp:
>> 
>> 159 int TypeStackSlotEntries::compute_cell_count(Symbol* signature, int max) {
>> 160   int args_count = 0;
>> 161   ResourceMark rm;
>> 162   SignatureStream ss(signature);
>> 163   args_count += MIN2(ss.reference_parameter_count(), max);
>> 164   return args_count * per_arg_cell_count;
>> 165 }
>> 
>> Why are you using args_count += now?  Before you had:
>> 
>> 164   ResourceMark rm;
>> 165   SignatureStream ss(inv.signature());
>> 166   int args_count = MIN2(ss.reference_parameter_count(), max);
> 
> Same as above. Will clean it up.
> 
>> 
>> ! void TypeStackSlotEntries::post_initialize(Symbol* signature, bool has_receiver) {
>>   ResourceMark rm;
>> +   int start = 0;
>> +   ArgumentOffsetComputer aos(signature, _nb_entries-start);
>> +   aos.total();
>> +   for (int i = start; i < _nb_entries; i++) {
>> +     set_stack_slot(i, aos.off_at(i-start) + (has_receiver ? 1 : 0));
>> +     set_type(i, type_none());
>> +   }
>> + }
>> 
>> I don't see start being changed.  Any reason to have it in a local?  If you want to keep it for clarity make it a const int.
> 
> Again same as above.

I can see a pattern here :-)

> 
>> src/share/vm/oops/methodData.hpp:
>> 
>> 753 protected:
>> 754   const int _nb_entries;
>> 
>> I'd advocate for not abbreviating to ambiguous letters.  Use the word you wanted to use.
> 
> Ok.
> 
> Roland.
> 
>> 
>> On Oct 8, 2013, at 9:22 AM, Roland Westrelin <roland.westrelin at oracle.com> 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.
>> 
>