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

[Vladimir Ivanov]

My idea was to warn Zero maintainers about a change which breaks their 
code and give them some time to react accordingly (provide missing 
functionality). After that just push the change. If Zero support is 
missing, then just file a bug.

If there's an RFE filed and Zero maintainers promise to fix it in a 
prompt manner, I'm fine with what you proposed.

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.

Best regards,
Vladimir Ivanov

On 11/5/15 7:22 PM, Aleksey Shipilev wrote:
> Isn't the incomplete (for the sake of Zero) M::is_accessor is a shared
> code pollution already? With that interpretation, the patch tries to
> contain the pollution in interpreter.cpp, with M::is_simple_accessor.
>
> I have already asked zero-dev here, and linked a response below:
>    http://mail.openjdk.java.net/pipermail/zero-dev/2015-November/000551.html
>
> The only way to produce a completely pure change is to have Zero changes
> along with M::is_accessor changes (which seem to be small, but require
> testing anyhow). Is that what you want? My concern is that we are
> predicating the tech debt removal on a code which we don't build or
> test. It seems that carefully containing the Zero-specific behavior, and
> letting Zero maintainers know to sweep it up is the sane tactics here.
>
> Thanks,
> -Aleksey
>
> On 11/05/2015 05:07 PM, Vladimir Ivanov wrote:
>> Overall, looks good.
>>
>> How much work is it required on Zero side to make it work? It would be
>> good to avoid shared code pollution (even temporal) just for Zero
>> purposes. My concern is that it will stay that way forever.
>>
>> If it is too much work for you, I'd ask Zero maintainers for help and
>> coordinate the changes.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 11/3/15 6:20 PM, Aleksey Shipilev wrote:
>>> Hi,
>>>
>>> I would like to have a formal review for a minor nit in
>>> Method::is_accessor. The definition for this method is inconsistent with
>>> its intent: it should accept all accessors, but instead it only  accepts
>>> the specific shapes of getters, and completely ignores setters. See:
>>>      https://bugs.openjdk.java.net/browse/JDK-8140650
>>>
>>> This makes compilers to ignore many trivial methods that we might
>>> otherwise inline when all other inline hints have failed. It seems to be
>>> a lingering issue left from interpreters that had the "fast accessors".
>>> While it is an open question should inlining policy treat accessors
>>> differently or not (I stand by "yes, it should"), this is a fix that
>>> makes is_accessors proper:
>>>      http://cr.openjdk.java.net/~shade/8140650/webrev.01/
>>>
>>> The only usage for the "old" style is_accessor is Zero, and they would
>>> like to update them after we commit the change:
>>>
>>> http://mail.openjdk.java.net/pipermail/zero-dev/2015-November/000551.html
>>>
>>> The patch passes JPRT, RBT (hotspot_all), and the new regression test.
>>>
>>> It beats me whether this is a runtime, or compiler change -- JIRA bug
>>> flip-flops on that -- so, sending to hotspot-dev at . I think it can be
>>> pushed via hs-comp, given it impacts compilers mostly, and has the
>>> compiler-specific test.
>>>
>>> Thanks,
>>> -Aleksey
>>>
>>>
>
>