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

[Aleksey Shipilev]

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.

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