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

[Vladimir Ivanov]

Vladimir K.,

Thanks for looking into this.
See my answers inlined.

Updated webrev: http://cr.openjdk.java.net/~vlivanov/8001107/webrev.03/

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