review(S): 7058510: multinewarray with 6 dimensions uncommon traps in server compiler

[Vladimir Kozlov]

Peter B. Kessler wrote:
> In src/share/vm/opto/parse3.cpp:478, you took out the default for the 
> switch, which used to catch the case where ndimensions==1 and call 
> ShouldNotReachHere().  Now, if ndimension==1 you will go to the new 
> OptoRuntime::multianewarrayN_* methods.  I think that will work (but I 
> haven't checked carefully), but I wonder if that's what you intended, or 
> if we shouldn't send that case to new_array, or call ShouldNotReachHere 
> because something really is wrong in that case?

The code at line 448 process ndimensions==1 arrays.

> 
> I understand the principle of least change, especially at this point a 
> release, but do we really still need the special cases for arrays of 2, 
> 3, 4, and 5 dimensions?  Should there be another bug to clean those up 
> later.  They are extra code to maintain.

New code allocate new java array so it is slower and can trigger other events 
(more GC collections) than previous code so we decided to keep it.

Vladimir

> 
> I don't know the opto/runtime style, but there sure seems to be a lot of 
> copying of the array dimensions.  Could you make up the jint array early 
> and just pass that around, or do you need the Node array of intcon's.  
> (E.g., do you need them for the type mechanisms?)  This is more a 
> question for my understanding than a comment on your code.
> 
>             ... peter
> 
> On 20110701 12:55 PM, Igor Veresov wrote:
>> Following the advise of John Rose I made arguments transfer via java 
>> array for arrays with > 5 dimensions.
>>
>> Webrev: http://cr.openjdk.java.net/~iveresov/7058510/webrev.02/
>>
>> igor
>>
>>
>> On 6/27/11 2:47 PM, Igor Veresov wrote:
>>> Thanks for the review, Tom!
>>> I'll try to implement the vararg calls.
>>>
>>> igor
>>>
>>> On 6/27/11 1:41 PM, Tom Rodriguez wrote:
>>>> Is there any easy way to just make C2 support a varargs style call for
>>>> multianewarray? C1 just does a funny calling convention where it
>>>> passes a pointer a stack allocated array which is obviously possible
>>>> with C2 though we don't have anything like it right now. I can see how
>>>> to do this by cons'ing up type signatures as need with extra count and
>>>> pointer arguments and then modifying the c_calling_convention to
>>>> switch to the stack only calling convention after the first two. The
>>>> only extra trick would be getting the address of the stack arguments
>>>> into the call when it's converted into a MachCall. We don't really
>>>> have a way to express that using MachNodes since stack slots are
>>>> virtual are that point. Maybe it could simply be handled at the ad
>>>> file level.
>>>>
>>>> So something like a new MachCallRuntimeVarargsNode with an index
>>>> indicating which argument is the last real one, the
>>>> c_calling_convention modified to take an index indicating when it
>>>> should force a switch to stack only, and the ins_encode for that match
>>>> inserting the right address computation for the address. Is that too
>>>> complex for the limited benefit? Maybe compared to the complexity of
>>>> the existing machinery, it's not too bad.
>>>>
>>>> As far as your webrev, I don't think you want to do this:
>>>>
>>>> + if (per_bc_reason == Reason_unhandled) {
>>>> + make_not_compilable = true;
>>>> + }
>>>>
>>>> There are other uses of Reason_unhandled and it's not clear the
>>>> stopping compilation is the right action for those other cases. Maybe
>>>> you need another action? Or maybe the actions need to be interpreted
>>>> specially for Reason_unhandled. Doing a make_not_entrant doesn't seem
>>>> like the right action for some of the other Reason_unhandleds.
>>>>
>>>> I do like the trap_state changes if it allows us to move
>>>> Reason_loop_predicate and Reason_loop_limit_check into the per bci
>>>> section.
>>>>
>>>> tom
>>>>
>>>> On Jun 27, 2011, at 12:07 PM, Igor Veresov wrote:
>>>>
>>>>> Problem: multinewarray with>= 6 dimensions is not supported by c2 and
>>>>> is plugged with uncommon trap. When executed in the main code path
>>>>> this yields performance far worse than even interpreter.
>>>>>
>>>>> Solution: Count these traps per bci and when the number exceeds
>>>>> PerBytecodeTrapLimit make it not compilable. With tiered it would
>>>>> result in recompilation at level 1 (pure C1), with regular scheme the
>>>>> method will continue in the interpreter.
>>>>>
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/7058510/webrev.00/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> igor
>>>>
>>>
>>
>