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

[Aleksey Shipilev]

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