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

[Christian Thalinger]

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.

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.

+   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?

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:

+  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.

src/share/vm/ci/ciMethodData.hpp:

+   // If the compiler finds a profiled type is known statically for

"type that is known"

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++;

+   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().

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

Why 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.

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.

+     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.

src/share/vm/runtime/globals.hpp:

+   develop(bool, PrintMethodDataStats, false,                                \

Most of these flags are named *Statistics.

+   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.

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.
> 
>