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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Mon Sep 2 23:20:02 PDT 2013


Thank you, Vladimir.

Chris, Rickard, are you ok with the latest version?

Best regards,
Vladimir Ivanov

On 9/3/13 6:14 AM, Vladimir Kozlov wrote:
> 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
>>>>>>>>>>>>
>>>>>>>>>


More information about the hotspot-compiler-dev mailing list