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

Peter B. Kessler Peter.B.Kessler at Oracle.COM
Fri Jul 1 14:01:02 PDT 2011

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?

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.

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 ccallingconvention 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 ccallingconvention modified to take an index indicating when it should force a switch to stack only, and the insencode 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 (perbcreason == Reasonunhandled) { + makenotcompilable = true; + } There are other uses of Reasonunhandled 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 Reasonunhandled. Doing a makenotentrant doesn't seem like the right action for some of the other Reasonunhandleds. I do like the trapstate changes if it allows us to move Reasonlooppredicate and Reasonlooplimitcheck 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

More information about the hotspot-compiler-dev mailing list