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

[Vladimir Kozlov]

On 8/30/13 7:07 AM, Vladimir Ivanov wrote:
> Vladimir K.,
>
> Thanks for looking into this.
> See my answers inlined.
>
> Updated webrev: http://cr.openjdk.java.net/~vlivanov/8001107/webrev.03/

I think this is good. The only concern left is object arrays with 
compressed or uncompressed oops. Please, test both cases (coop on and off).

About test. I would suggest to split it per type. I had the same 
situation with vectors testing and got better coverage with separate 
tests. For example, you only tests IntArrayArity but you need to test 
all primitive type arrays.

Why you don't test StaticObjectStable and ObjectArrayArity2? 
ObjectArrayArity2 can't be stable but we still need to test it. Also why 
you start obj arrays test names with 0, ObjectArrayArity0, when 
IntArrayArity1.

thanks,
Vladimir

>
> Best regards,
> Vladimir Ivanov
>
>>>> 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.
> Ok. I removed this change. Any objections from other reviewers?
>
>>>> 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 {
> Added as GraphKit::cast_array_to_stable.
>
>>>> 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.
> Done. Testing in progress.
>
>>>
>>> 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.
>
> Filed JDK-8024042: "Add verification support for @Stable into VM" to
> track this.
>
>> 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
>>>>>>>>>
>>>>>>