Request for review (S): 7022998: JSR 292 recursive method handle calls inline themselves infinitely

[Christian Thalinger]

On Mar 18, 2011, at 6:41 PM, Vladimir Kozlov wrote:
> I would better keep "made zombie" and "made not entrant" than new z/e flags.

Alright.  I removed them.  -- Christian

> 
> Vladimir
> 
> Christian Thalinger wrote:
>> On Mar 18, 2011, at 6:03 PM, Tom Rodriguez wrote:
>>> compileBroker.hpp:
>>> 
>>> You can't call get_methodOop here.
>>> 
>>> +   static void  print_inlining(ciMethod* method, int inline_level, int bci, const char* msg = NULL) {
>>> +     print_inlining(tty, method->get_methodOop(), inline_level, bci, msg);
>>> +   }
>>> 
>>> since you are in native mode.  You have to transition to VM or you have rearrange it so that you can use the ciMethod methods to unpack it directly.  You might be able to get away with templating it since I think ciMethod and methodOop have similar calling interfaces.
>> I thought I get into trouble using methodOop directly here.  Templating is a good idea but there is a small difference:
>> methodOopDesc::has_exception_handler
>> vs.
>> ciMethod::has_exception_handlers
>>> compileBroker.cpp:
>>> 
>>> I think you can just lock MethodCompileQueue_lock directly instead of indirecting through the queue.
>> OK.
>>> Remove the is_native assert.
>> OK.
>>> I don't like the z/e flag since we already print the whole thing follow it.
>> How about removing the following output?  The z/e flag is better to see than some unaligned output at the end of the line.
>> -- Christian
>>> Overall it looks very good.
>>> 
>>> tom
>>> 
>>> On Mar 18, 2011, at 9:18 AM, Christian Thalinger wrote:
>>> 
>>>> On Mar 17, 2011, at 7:19 PM, Tom Rodriguez wrote:
>>>>>>> Native wrappers are never inlined.  That would be a intrinsic in the output you show above.  Intrinsic inlining should always have a bci though.
>>>>>> Ahh, right, I mixed them.  When I'm already at it, should I change the intrinsic output too:
>>>>>> 
>>>>>> Inlining intrinsic _min at bci:67 in sun.nio.cs.UTF_8$Encoder::encodeArrayLoop (489 bytes)
>>>>>> 
>>>>>> to fit into the inlining tree?
>>>>> I wouldn't mind that.  Something like the normal output but with "(intrinsic)" at the end?
>>>> Alright, I changed a lot more than I wanted to but now it seems much better and clearer, at least to me.
>>>> 
>>>> I removed nmethod::print_compilation completely (which was called from CompileTask::print_compilation) and moved all printing logic into CompileTask.
>>>> 
>>>> http://cr.openjdk.java.net/~twisti/7022998/
>>>> 
>>>> Some output looks a little different now, like instrinsics (as we talked about):
>>>> 
>>>> 1111    3             java.lang.String::lastIndexOf (68 bytes)
>>>>                          @ 26   java.lang.Math::min (11 bytes)   (intrinsic)
>>>> 
>>>> To be able to remove some utterly complex code (the title code from nmethod::print_compilation), made_not_entrant and made_zombie messages are now printed and the end of the line (like all other status messages).  Additionally I added a status flag (e = not entrant, z = zombie):
>>>> 
>>>> 4006  150      z 4       java.lang.String::equals (88 bytes)   made zombie
>>>> 4565  121      e 3       java.util.HashMap::get (79 bytes)   made not entrant
>>>> 
>>>> Type profile output looks like:
>>>> 
>>>>    @ 121   java.util.HashMap::addEntry (58 bytes)   inline (hot)
>>>>     \-> TypeProfile (6020/6257 counts) = java/util/HashMap
>>>> 
>>>> or:
>>>> 
>>>>    @ 89   java.util.LinkedHashMap$Entry::recordAccess (35 bytes)   inline (hot)
>>>>     \-> TypeProfile (13/131 counts) = java/util/LinkedHashMap$Entry
>>>>     \-> TypeProfile (118/131 counts) = java/util/HashMap$Entry
>>>> 
>>>> Native wrapper compiles now look like this (I print a "-" instead of the compile level for tiered):
>>>> 
>>>> 2077    0     n  -       java.lang.System::nanoTime (0 bytes)   (static)
>>>> 
>>>> But the output might look a little odd since the native wrapper output may be between compile output and inline output looking like the native wrapper has an inlinee:
>>>> 
>>>> 2102   69        3       java.lang.CharacterDataLatin1::toUpperCaseEx (71 bytes)
>>>> 2102    0     n  -       sun.misc.Unsafe::compareAndSwapInt (0 bytes)                                @ 4   java.lang.CharacterDataLatin1::getProperties (11 bytes)
>>>> 
>>>> About the compile id for native wrappers, assign_compile_id has an assert:
>>>> 
>>>> assert(!method->is_native(), "no longer compile natives");                                                                                                             
>>>> So either we remove that assert or we don't assign a compile id to native wrapper compiles.
>>>> 
>>>> Let me know what you think.
>>>> 
>>>> -- Christian
>>>> 
>>>> PS: The original bug fix is still in there :-)