Preliminary RFR (L): 8001107: @Stable annotation for constant folding of lazily evaluated variables

[Vladimir Ivanov]

Vladimir K.,

Thank you for review!

> java/lang/invoke/MethodHandle.java has 0 diffs.
It's a bug in webrev - I have multiple mq patches and the changes in 
this file are reverted.

> ciTypeArray.cpp: why we need these changes?
I consider this as a cleanup. element_value(index).as_char() performs 
additional checks and it looks better (IMO, of course :-) ) than 
get_typeArrayOop()->char_at(index).

Looking at the code now, I think assert is redundant there. I'll remove it.

> compile.cpp add {}:
>
> +       if (flat->offset() == TypePtr::OffsetBot)
> +         alias_type(idx)->set_element(flat->is_aryptr()->elem());
Fixed.

>
> UseImplicitStableValues code is repeated 2 times. Could be moved into
> separate method.
It would be nice, but I'm not sure where to place such utility method 
(one usage in GraphKit and the other one in LibraryCallKit).

> memnode.cpp: Why in fold_stable_ary_elem() offset is taking from
> ary->offset() and nor from adr?
It's an artifact of factoring fold_stable_ary_elem out of 
LoadNode::Value where offset is used before we start looking at inputs.

Do you suggest to extract offset from adr->in(AddPNode::Offset) node's 
type?

I can add an assert to verify that both offset values are equal.
BTW, I tried to extract offset value from adr, but the code becomes more 
complicated. Do I miss a better way to extract the value from a Con[IL]Node?

#ifdef ASSERT
     const Type* t = adr->in(AddPNode::Offset)->as_Type()->type();
     switch(t->basic_type()) {
       case T_INT:  assert(t->isa_int()->get_con()  == off, "offset 
mismatch"); break;
       case T_LONG: assert(t->isa_long()->get_con() == off, "offset 
mismatch"); break;
       default: fatal(err_msg_res("Unexpected type: %d", t->basic_type()));
     }
#endif // ASSERT

> Can we have an assert in template interpreter that with FoldStableValues
> we only do 0/NULL -> non_0/not_NULL transition for @stable values?
We can. I quickly prototyped [1] something similar for x86_64 (sigh, I 
couldn't avoid touching platform-specific code). It doesn't look that 
ugly. However, it seems better to implement such verification as JVMTI 
agent.

Best regards,
Vladimir Ivanov

[1] http://cr.openjdk.java.net/~vlivanov/8001107/assert.00/

> In general seems fine.
>
> Thanks,
> Vladimir
>
> On 8/15/13 2:05 AM, Vladimir Ivanov wrote:
>> Chris, thank you for the review.
>>
>> I need one more review from a person wearing Reviewer hat :-) Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 8/13/13 9:08 PM, Christian Thalinger wrote:
>>> Looks good.  -- Chris
>>>
>>> On Jul 26, 2013, at 2:53 PM, Vladimir Ivanov
>>> <vladimir.x.ivanov at oracle.com> wrote:
>>>
>>>> Updated according to comments from Chris:
>>>> http://cr.openjdk.java.net/~vlivanov/8001107/webrev.02
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> On 7/23/13 9:31 PM, Vladimir Ivanov wrote:
>>>>> Rickard,
>>>>>
>>>>> Thank you for review.
>>>>> See my answers inline.
>>>>>
>>>>> Updated version:
>>>>> http://cr.openjdk.java.net/~vlivanov/8001107/webrev.01
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> On 7/23/13 2:46 PM, Rickard Bäckman wrote:
>>>>>> Vladimir,
>>>>>>
>>>>>> first, nice work.
>>>>>>
>>>>>> A couple of comments about the code:
>>>>>>
>>>>>> ciArray.hpp / ciArray.cpp
>>>>>> Looks like most of the new methods could be declared const.
>>>>> ciObject::klass() isn't const, so newly introduced methods can't be
>>>>> made
>>>>> const as well.
>>>>>
>>>>>> ciConstant.hpp
>>>>>> I would like to have braces add to the if / else if / else
>>>>> Done.
>>>>>
>>>>>> classFileParser.hpp
>>>>>> The is_stable() should be declared const.
>>>>> Done.
>>>>>
>>>>>> memnode.cpp
>>>>>> You are adding plenty of logic to an already very long method, could
>>>>>> we extract the new parts to its own method? or methods.
>>>>> Done. Introduced fold_stable_ary_elem helper function.
>>>>>
>>>>>> The elembt local variable is never really used.
>>>>> Removed.
>>>>>
>>>>>> type.cpp
>>>>>> 2 instances of assert(stable == true || stable == false, ""); - this
>>>>>> seems to be always true?
>>>>> Agree. Removed.
>>>>>
>>>>>> The constant 42 in the hash, since we are hashing pointers it
>>>>>> might be
>>>>>> more effective to use and odd number?
>>>>> Done.
>>>>>
>>>>>> 3505: if (stable_dimension <= 0 || stable_dimension == 1 && stable ==
>>>>>> this->stable())
>>>>>> should have parantheses added to clarify the || && logic.
>>>>> Done.
>>>>>
>>>>>> 3511   if (stable_dimension > 1 && elem_ptr != NULL &&
>>>>>> elem_ptr->base() == Type::AryPtr) {
>>>>>> could use the is_aryptr() instead of comparing base to Type::AryPtr
>>>>> Done.
>>>>>
>>>>>> type.hpp
>>>>>> the getter is named stable(), in all other places it is is_stable()
>>>>>> maybe we could rename stable -> is_stable.
>>>>> Done.
>>>>>
>>>>>> Cheers
>>>>>> /R
>>>>>>
>>>>>> On Jul 20, 2013, at 12:19 AM, Vladimir Ivanov wrote:
>>>>>>
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8001107/
>>>>>>> 1455 lines changed: 1366 ins; 34 del; 55 mod;
>>>>>>>
>>>>>>> 8001107: @Stable annotation for constant folding of lazily evaluated
>>>>>>> variables
>>>>>>>
>>>>>>> Excerpt from Javadoc:
>>>>>>> [@Stable annotation is] Internal marker for some fields in the JSR
>>>>>>> 292 implementation. A field may be annotated as stable if all of its
>>>>>>> component variables changes value at most once. A field's value
>>>>>>> counts as its component value. If the field is typed as an array,
>>>>>>> then all the non-null components of the array, of depth up to the
>>>>>>> rank of the field's array type, also count as component values. By
>>>>>>> extension, any variable (either array or field) which has annotated
>>>>>>> as stable is called a stable variable, and its non-null or non-zero
>>>>>>> value is called a stable value.
>>>>>>>
>>>>>>> Testing: unit test, JPRT, VM testbase, nashorn, octane
>>>>>>>
>>>>>>> Contributed-by: jrose, vlivanov
>>>>>>>
>>>>>>> I marked review request as preliminary, because I think it may be
>>>>>>> too
>>>>>>> late to get it into 8. It'll be targeted for 8update then.
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>
>>>