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

[Christian Thalinger]

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