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

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 9/2/13 3:35 PM, Vladimir Ivanov wrote:
> Updated version: http://cr.openjdk.java.net/~vlivanov/8001107/webrev.04/
> See my answers inline.
>
> Best regards,
> Vladimir Ivanov
>
> On 8/30/13 9:27 PM, Vladimir Kozlov wrote:
>> 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).
> Good point. I added -XX:[+-]UseCompressedOops cases to all tests.
>
>> 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.
> Ok. I split tests per element type and added missing cases.
>
>> Why you don't test StaticObjectStable and ObjectArrayArity2?
> I had Object (arrays) static fields later in the test. But I added StaticObjectStable.
>
>> 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.
> ObjectArrayArity* test a situation when static & dynamic array dimensions mismatch. I decided to cover 2 cases:
>      - Object -> *[]      (#0)
>          - Object[] -> *[][]  (#1)
>
> Added 3rd case: Object[][] -> *[][][].
>
> Best regards,
> Vladimir Ivanov
>
>>
>> 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
>>>>>>>>>>>
>>>>>>>>