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

[Vladimir Kozlov]

On 8/28/13 6:19 AM, Vladimir Ivanov wrote:
> 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).

But element_value() is much more expensive! I don't like this change.

>
> Looking at the code now, I think assert is redundant there. I'll remove it.
>
>>
>> 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).

class LibraryCallKit : public GraphKit {

>
>> 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.

There is already 'off' in the caller so pass it instead of 'adr' which 
is useless in fold_stable_ary_elem() because off != Type::OffsetBot 
check should be enough (verify that).

off < (uint)min_base_off could be done in caller which already has one - 
make it common.

>
> Do you suggest to extract offset from adr->in(AddPNode::Offset) node's
> type?
>
>> 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.

It is not bad. Please, keep it - file RFE and point to this webrev 
assert.00. Later we will decide how to do it. I am not fan of JVMTI 
because it does not run always.

Thanks,
Vladimir

>
> 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
>>>>>>>
>>>>