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

Mon Sep 2 15:35:55 PDT 2013

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