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

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

Igor Veresov wrote:
> John, Vladimir, Peter,
> 
> Thank you for your comments.
> I've made the changes and tested with my microbenchmark and DeoptimizeALot.
> 
> Webrev: http://cr.openjdk.java.net/~iveresov/7058510/webrev.03/
> 
> Thanks!
> igor
> 
> On 7/1/11 2:00 PM, Vladimir Kozlov wrote:
>> John Rose wrote:
>>> On Jul 1, 2011, at 1:26 PM, Vladimir Kozlov wrote:
>>>
>>>> Wrap new java dimension allocation into PreserveReexecuteState scope
>>>> and restore stack.
>>>
>>> Is this in case the initial allocation deoptimizes (due to some
>>> external event)? I suppose that should be tested with DeoptimizeALot.
>>
>> The allocation of dimension array may go slow path (no space left in
>> TLAB) into runtime and on return the method could be deoptimized so we
>> need restore stack so that interpreter can reexecute multinewarray
>> allocation. There are several places where we do this already (for
>> example, around the call to expand_multianewarray()).
>>
>> And yes, it should be tested with DeoptimizeALot.
>>
>> Vladimir
>>
>>>
>>> Igor, the extraction from the dope array bothers me:
>>> + jint *j_dims = (jint*)dims->base(T_INT);
>>> + jint *c_dims = NEW_RESOURCE_ARRAY(jint, len);
>>> + memcpy(c_dims, j_dims, sizeof(jint)*len);
>>>
>>> It is correct, but I'd rather see more use of Hotspot machinery, with
>>> stronger typing:
>>> + jint *j_dims = typeArrayOop(dims)->int_at_addr(0);
>>> + jint *c_dims = NEW_RESOURCE_ARRAY(jint, len);
>>> + Copy::conjoint_jints_atomic(c_dims, j_dims, sizeof(jint)*len);
>>>
>>> Note that Copy::* strongly outnumbers memcpy in the opto directory.
>>> The distinction is mostly stylistic, but Copy:: is more strongly typed
>>> in this case.
>>>
>>> (In other uses, Copy:: is more explicit and careful about some of the
>>> things we need to care about, such as overlaps and word-tearing.)
>>>
>>> Also, use is_typeArray instead of is_array in the previous assert.
>>> That will make the cast to typeArrayOop safe.
>>>
>>> -- John
>