RFR: aaload/aastore profiling

[Tobias Hartmann]

Hi Roland,

On 15.11.19 16:00, Roland Westrelin wrote:
> This?
> 
> diff -r f81c768ad125 src/hotspot/share/c1/c1_Runtime1.cpp
> --- a/src/hotspot/share/c1/c1_Runtime1.cpp	Wed Nov 13 17:51:40 2019 +0100
> +++ b/src/hotspot/share/c1/c1_Runtime1.cpp	Fri Nov 15 15:55:56 2019 +0100
> @@ -473,15 +473,16 @@
>  
>  
>  JRT_ENTRY(void, Runtime1::store_flattened_array(JavaThread* thread, valueArrayOopDesc* array, int index, oopDesc* value))
> -  assert(ArrayKlass::cast(array->klass())->storage_properties().is_flattened() || (ArrayKlass::cast(array->klass())->storage_properties().is_null_free() && value == NULL), "should not be called");
>    if (ArrayKlass::cast(array->klass())->storage_properties().is_flattened()) {
>      profile_flat_array(thread);
>    }
>  
>    NOT_PRODUCT(_store_flattened_array_slowcase_cnt++;)
>    if (value == NULL) {
> +    assert(ArrayKlass::cast(array->klass())->storage_properties().is_flattened() || ArrayKlass::cast(array->klass())->storage_properties().is_null_free(), "should not be called");
>      SharedRuntime::throw_and_post_jvmti_exception(thread, vmSymbols::java_lang_NullPointerException());
>    } else {
> +    assert(ArrayKlass::cast(array->klass())->storage_properties().is_flattened(), "should not be called");
>      array->value_copy_to_index(value, index);
>    }
>  JRT_END

My bad, that doesn't make it easier to read. Leave it as is.

>> - ciObjArrayKlass.cpp: why are these checks required?
> 
> A non null free value array has a subtype, the null free value array,
> right?
> Without that change, the c1 code assumes that if it sees any value
> array, it knows the exact type.

Okay, I was just wondering why we didn't need that code before.

>> - Should the C1 and interpreter profiling be guarded by EnableValhalla?
> 
> The problem is that I replaced the existing aastore profiling. So with
> -EnableValhalla we would go back to the previous aastore profiling and
> previous MonomorphicArrayCheck?

Right if that's getting too complex, we can just have it always enabled.

>> I've executed some testing, all passed! How did you verify performance?
> 
> Easy. I didn't. It obviously improves performance! :-)

Okay :) Maybe Sergey can do some evaluation once this is in.

>> Please file an enhancement for this so we can better keep track of related tasks.
> 
> What you're saying is I need a bug id for this change, right?

Yes.

It would be good if someone from runtime could look at this as well.

Best regards,
Tobias