RFR (S) 8140650: Method::is_accessor should cover getters and setters for all types

[Christian Thalinger]

> On Nov 5, 2015, at 10:52 AM, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:
> 
> Thanks for the review, Coleen!
> 
> I'll wait for RBT results to come in, and if they are positive, I'll
> push (I think through hs-comp, since it affects compilers mostly).
> 
> TODO in interpreter.cpp serves as the anchor for the upcoming Zero RFE.

Was one filed?

> 
> Cheers,
> -Aleksey
> 
> On 11/05/2015 11:42 PM, Coleen Phillimore wrote:
>> 
>> Our emails crossed.   This change looks good.  I don't know how
>> necessary the Zero TODO is but the comment is fine.
>> 
>> Coleen
>> 
>> On 11/5/15 3:16 PM, Aleksey Shipilev wrote:
>>> Ah, no, the CallNode::may_modify polls is_accessor only for boxed
>>> instances, so there are no setters in Java code. Still, we should check
>>> for getters there explicitly, and the getter/setter split is sensible in
>>> Method API anyhow.
>>> 
>>> New webrev:
>>>   http://cr.openjdk.java.net/~shade/8140650/webrev.02/
>>> 
>>> Re-spinning RBT for sanity.
>>> 
>>> Thanks,
>>> -Aleksey
>>> 
>>> On 11/05/2015 10:45 PM, Aleksey Shipilev wrote:
>>>> Thanks, Coleen!
>>>> 
>>>> The tricky part is that compilers poll Method::is_accessor via:
>>>> 
>>>>  bool ciMethod::is_accessor    () const {
>>>>    FETCH_FLAG_FROM_VM(is_accessor);
>>>>  }
>>>> 
>>>> #define FETCH_FLAG_FROM_VM(flag_accessor) { \
>>>>   check_is_loaded(); \
>>>>   VM_ENTRY_MARK; \
>>>>   return get_Method()->flag_accessor(); \
>>>> }
>>>> 
>>>> ...which may be opaque for IDEs, when you look for usages. I found that
>>>> a text search for "is_accessor" yields only positive matches.
>>>> 
>>>> *Actually*, now that I read the code again, there is a suspicious use in
>>>> CallNode::may_modify:
>>>> 
>>>>       if (meth->is_accessor()) {
>>>>         return false;
>>>>       }
>>>> 
>>>> This seems to break if is_accessor accepts setters. Let me split the
>>>> is_accessor into is_getter/is_setter and re-run tests. If you spot other
>>>> problems in the patch, let me know.
>>>> 
>>>> Thanks,
>>>> -Aleksey
>>>> 
>>>> On 11/05/2015 08:34 PM, Coleen Phillimore wrote:
>>>>> I am planning on reviewing this but I haven't had time.  And I'm trying
>>>>> to build Zero for a different reason.  I would be pretty unhappy if it
>>>>> broke Zero and would like it to not do that.  I haven't had a chance to
>>>>> read all of this yet.  I thought is_accessor was only for Zero so I
>>>>> guess I have to read more why this is getting changed.  I will try to
>>>>> get to it today.
>>>>> 
>>>>> Thanks,
>>>>> Coleen
>>>>> 
>>>>> On 11/5/15 12:24 PM, Aleksey Shipilev wrote:
>>>>>> On 11/05/2015 07:59 PM, Vladimir Ivanov wrote:
>>>>>>>>> If there's an RFE filed and Zero maintainers promise to fix it in a
>>>>>>>>> prompt manner, I'm fine with what you proposed.
>>>>>>>> Yeah, that's the plan.
>>>>>>>> 
>>>>>>>>> Also, I had an idea: why don't you move is_simple_accessor() from
>>>>>>>>> Method
>>>>>>>>> to some Zero-specific location? I don't see any value in keeping
>>>>>>>>> it in
>>>>>>>>> Method.
>>>>>>>> That's an interesting trick, but I think it will cause more harm:
>>>>>>>> shared
>>>>>>>> interpreter.cpp would have to know about that Zero-specific
>>>>>>>> method, or
>>>>>>>> otherwise intercept "too wide" accessor shape and narrow it down
>>>>>>>> before
>>>>>>>> it reaches Zero. Therefore, I believe is_simple_accessor is the
>>>>>>>> lesser
>>>>>>>> evil.
>>>>>>> Ok. Looks good then.
>>>>>> Thanks for taking time to review, Vladimir! I'll count that as the
>>>>>> review from the compiler side.
>>>>>> 
>>>>>> I think we still need a review from the runtime side. Folks? For the
>>>>>> record, we are discussing this change:
>>>>>>    http://cr.openjdk.java.net/~shade/8140650/webrev.01/
>>>>>> 
>>>>>> The patch passes JPRT, RBT (hotspot_all), and the new regression test.
>>>>>> 
>>>>>> Thanks,
>>>>>> -Aleksey
>>>>>> 
>>>>>> 
>>>> 
>>> 
>> 
> 
>