RFR (S) 8140650: Method::is_accessor should cover getters and setters for all types
aleksey.shipilev at oracle.com
Thu Nov 5 16:22:58 UTC 2015
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