RFR(L): 8023657: New type profiling points: arguments to call

[Roland Westrelin]

Thanks Chris for taking a look at this.

On Oct 3, 2013, at 9:38 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:

> src/share/vm/c1/c1_GraphBuilder.cpp:
> 
> + Values* GraphBuilder::args_list_for_profiling(ciMethod* target, int& start, bool may_have_receiver) {
> + Values* GraphBuilder::collect_args_for_profiling(Values* args, ciMethod* target, bool may_have_receiver) {
> 
> Both methods take a target as argument but never use it.

It's used in a subsequent patch. Thanks for spotting that. I'll remove it.

> src/share/vm/c1/c1_Instruction.hpp:
> 
> +     bool arg_needs_null_check(int i) const {
> +       if (i >= 0 && i < (int)sizeof(_nonnull_state) * BitsPerByte) {
> 
> This means that arguments after the 32nd position always need a null check.  Can you add a comment to these methods explaining that as we do for Java API methods?  Maybe use Doxygen style.

Ok.

> 
> +   virtual void input_values_do(ValueVisitor* f)   {
> +     if (_recv != NULL) f->visit(&_recv);
> +     for (int i = 0; i < nb_profiled_args(); i++) f->visit(_obj_args->adr_at(i));
> +   }
> 
> Can you use some line breaks and curly braces here too?

Ok.

> src/share/vm/c1/c1_LIR.hpp:
> 
> +   intptr_t     _current_klass; // what the profiling currently reports
> 
> Why is the type intptr_t?  Can't we use a more specific type here?  Hmm, you tag klass pointers:

Indeed.

> 
> +  static intptr_t with_status(intptr_t k, intptr_t in) {
> +    return k | (in & status_bits);
> +  }
> 
> src/share/vm/c1/c1_LIRGenerator.cpp:
> 
> + static ciKlass* sig_next_klass(ciSignature* sig, ciKlass*& first_sig_k, int& j) {
> 
> We should move this method into ciSignature.  Or better ciSignatureStream.

I thought about it but figured it was too specific to be useful to anything else. Anyway, I can make that change.

> 
> src/share/vm/ci/ciMethodData.hpp:
> 
> +   // If the compiler finds a profiled type is known statically for
> 
> "type that is known"

Ok.

> 
> src/share/vm/oops/methodData.cpp:
> 
> +   SignatureStream ss(inv.signature());
> +   int args_count = 0;
> +   for ( ; !ss.at_return_type(); ss.next()) {
> +     if (ss.is_object() || ss.is_array()) {
> +       args_count++;
> +       if (args_count >= max) {
> +         break;
> +       }
> +     }
> +   }
> 
> This would be a nice utility method in SignatureStream (sans the >= max part).  Maybe reference_parameter_count().  Oh, btw., arrays are treated as objects:
> 
> bool SignatureStream::is_object() const {
>  return _type == T_OBJECT
>      || _type == T_ARRAY;
> }
> 
> It could be reused here right away:
> 
> + #ifdef ASSERT
> +   int count = 0;
> +   for (SignatureStream ss(inv.signature()); !ss.at_return_type(); ss.next()) {
> +     if (ss.is_object() || ss.is_array()) {
> +       count++;

Ok, I'll see what I can do.

> 
> +   Bytecodes::Code bc = m->code_at(bci);
> +   assert(Bytecodes::is_invoke(m->java_code_at(bci)), "arguments only make sense at call");
> + 
> +   return bc == Bytecodes::_invokedynamic || bc == Bytecodes::_invokehandle;
> 
> Instead of this code you can use Bytecode_invoke and then query inv.is_invokedynamic() || inv.is_invokehandle().

Sure.

> 
> + int MethodData::profile_arguments_flag() {
> +   return TypeProfile % 10;
> + }
> 
> Why 10?

That's also something that leaked from a subsequent patch. I intend to use TypeProfile=xyz, with x,y,z in {0,1,2} to select the level of profiling for each of arguments, parameters, return value. I welcome other suggestions. I'll remove the % 10.

> 
> + bool MethodData::profile_arguments() {
> +   return profile_arguments_flag() > 0 && profile_arguments_flag() <= 2;
> + }
> + 
> + bool MethodData::profile_arguments_jsr292_only() {
> +   return profile_arguments_flag() == 1;
> + }
> + 
> + bool MethodData::profile_all_arguments() {
> +   return profile_arguments_flag() == 2;
> + }
> 
> I think you should have an enum for all of these constants.

Ok.

> 
> src/share/vm/oops/methodData.hpp:
> 
> +   void set_int_at_unchecked(int index, int value) {
> +     data()->set_cell_at(index, (intptr_t) value);
> +   }
> 
> This method is not used.

Well you're much better than I am at spotting unused stuff. I'll remove it.

> 
> +     return _args.set_type(i, TypeEntries::with_status((intptr_t)k, current));
> 
> Maybe you should add another with_status method that takes a Klass* and do the cast there.

Ok.

> 
> src/share/vm/runtime/globals.hpp:
> 
> +   develop(bool, PrintMethodDataStats, false,                                \
> 
> Most of these flags are named *Statistics.

I'll make the change.

> 
> +   product_pd(uintx, TypeProfile,                                            \
> +           "Type profiling of arguments at call:"                            \
> +           "0->off ; 1->js292 only; 2->all methods")                         \
> +   product(intx, TypeProfileArgsLimit,     2,                                \
> +           "max number of call arguments to consider for type profiling")    \
> +                                                                             \
> 
> Empty line missing between.

I'll add the missing like.

> 
> On Oct 2, 2013, at 10:02 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
>> Here is a new webrev for the new profiling points, only profiling of arguments at calls this time. This one includes tiered support on 32bit and 64bit x86.
>> 
>> http://cr.openjdk.java.net/~roland/8023657/webrev.01/
>> 
>> The output of -XX:+PrintMethodData is:
>> 
>> 6 invokevirtual 2 <TestProfiling.m2(LTestProfiling$C;Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;>
>> 0   bci: 6    VirtualCallData     count(0) entries(1)
>>                                   'TestProfiling'(4746 1.00)
>>               argument types      0: stack(1) 'TestProfiling$C'
>>                                   1: stack(2) unknown (null seen)
>> 
>> Profiling can either be off, on only for jsr292 related calls (invokedynamic, invokehandle or all invokes in a lambda form) or on for all methods and invokes. -XX:TypeProfile={0,1,2} is used to pick a level of profiling.
>> Once all profiling is in (arguments, parameters, return value), in order to limit the number of options my plan is to use TypeProfile=xyz, with x,y,z in {0,1,2} to select the level of profiling for each. TypeProfile=111, Typeprofile=222 etc.
>> Profiling is by default on only for jsr292 related calls in order to not impact startup/footprint of standard java apps.
>> 
>> Roland.
>> 
>> 
>