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

[Aleksey Shipilev]

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