RFR (S) 8140650: Method::is_accessor should cover getters and setters for all types
vladimir.x.ivanov at oracle.com
Thu Nov 5 16:46:38 UTC 2015
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
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:
> 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.
> 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:
>>> 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:
>>> 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:
>>> The only usage for the "old" style is_accessor is Zero, and they would
>>> like to update them after we commit the change:
>>> 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.
More information about the hotspot-dev