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

[Tom Rodriguez]

On Mar 17, 2011, at 10:35 AM, Christian Thalinger wrote:

> On Mar 17, 2011, at 6:31 PM, Tom Rodriguez wrote:
>> 
>> On Mar 17, 2011, at 4:25 AM, Christian Thalinger wrote:
>> 
>>> On Mar 15, 2011, at 7:41 PM, Vladimir Kozlov wrote:
>>>> Christian Thalinger wrote:
>>>>> On Mar 15, 2011, at 5:57 PM, Vladimir Kozlov wrote:
>>>>>> Christian,
>>>>>> 
>>>>>> Instead of duplicating code in several places could you factor it in one method?
>>>>> Actually I wanted to do that but I didn't know where to put the code.  sharedRuntime?
>>>> 
>>>> May be a static method in CompileTask or AbstractCompiler since it is compilation information.
>>> 
>>> CompileTask seems suitable.  I put a couple of static methods in there and the code is much cleaner now.  I updated the webrev.
>>> 
>>>> 
>>>>>> sharedRuntime.cpp: you removed "---" which was indication of native method wrapper compilation.
>>>>> Is there a difference between the "n" printed in nmethod::print_compilation and the "n" printed in AdapterHandlerLibrary::create_native_wrapper?
>>>> 
>>>> I never saw nmethod::print_compilation using 'n' in output. But I assume it is the same.
>>> 
>>> So I'd say we just drop the "---" and use "n" as the indicator for a native method wrapper.
>> 
>> I'm not sure I understand this conversation.  What --- are you dropping?  
> 
> Vladimir means the "---" which (I think) were originally a placeholder for the compile id.

Oh, right.  It would be nice if the formatting of the native wrapper wasn't so different from the regular compiles.  We could actually assign them compile ids.  It could be passed into create_native_wrapper and generated using assign_compile_id.  That would also fix the weird usage of print_inlining to describe native wrappers in your webrev that I hadn't noticed before.  You'd probably want to tweak the native wrapper nmethod constructor to take a compile_id again too.  This change is spiraling a bit but I think it's a good cleanup of the output.

tom

> 
>> n has always represented native wrappers.
> 
> True.  I missed that.
> 
> -- Christian
> 
>> 
>> tom
>> 
>>> 
>>> -- Christian
>>> 
>>>> 
>>>> Vladimir
>>>> 
>>>>>> Main fix is fine.
>>>>> Okay.
>>>>> -- Christian
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> Christian Thalinger wrote:
>>>>>>> http://cr.openjdk.java.net/~twisti/7022998/
>>>>>>> 7022998: JSR 292 recursive method handle calls inline themselves infinitely
>>>>>>> Reviewed-by:
>>>>>>> Methods doing recursive method handle calls, like in JRuby's
>>>>>>> bench_fib_resursive.rb or bench_tak.rb, inline themselves infinitely.
>>>>>>> This results in a huge method which hits compile thresholds and aborts
>>>>>>> inlining resulting in poor performance.
>>>>>>> The inlining logic needs to know about method handle call sites and
>>>>>>> search the call stack for recursive calls.
>>>>>>> This change also cleans up the PrintCompilation, PrintInlining and
>>>>>>> TraceTypeProfile output to be aligned since the tiered compilation
>>>>>>> changes added some additional output.
>>>>>>> src/share/vm/c1/c1_GraphBuilder.cpp
>>>>>>> src/share/vm/opto/bytecodeInfo.cpp
>>>>>>> src/share/vm/opto/doCall.cpp
>>>>>>> src/share/vm/runtime/sharedRuntime.cpp
> 
>