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

[Severin Gehwolf]

Hi Aleksey,

On Thu, 2015-11-05 at 23:52 +0300, Aleksey Shipilev wrote:
> Thanks for the review, Coleen!
> 
> I'll wait for RBT results to come in, and if they are positive, I'll
> push (I think through hs-comp, since it affects compilers mostly).
> 
> TODO in interpreter.cpp serves as the anchor for the upcoming Zero
> RFE.

Here is a partial implementation of the Zero bits. In fact, the code
was already there to handle other getters too. I don't know why it was
guarded with those asserts and with is_simple_accessor().

It does not yet implement the setter logic, but at least we don't have
to keep is_simple_accessor() around. The patch builds on top of your
latest 8140650/webrev.02/ webrev.

The patch I'm talking about is:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8140650/JDK-8140650-is_accessor_zero_cleanup.hotspot.patch

Feel free to incorporate it.

Cheers,
Severin

> Cheers,
> -Aleksey
> 
> On 11/05/2015 11:42 PM, Coleen Phillimore wrote:
> > 
> > Our emails crossed.   This change looks good.  I don't know how
> > necessary the Zero TODO is but the comment is fine.
> > 
> > Coleen
> > 
> > On 11/5/15 3:16 PM, Aleksey Shipilev wrote:
> > > Ah, no, the CallNode::may_modify polls is_accessor only for boxed
> > > instances, so there are no setters in Java code. Still, we should
> > > check
> > > for getters there explicitly, and the getter/setter split is
> > > sensible in
> > > Method API anyhow.
> > > 
> > > New webrev:
> > >    http://cr.openjdk.java.net/~shade/8140650/webrev.02/
> > > 
> > > Re-spinning RBT for sanity.
> > > 
> > > Thanks,
> > > -Aleksey
> > > 
> > > On 11/05/2015 10:45 PM, Aleksey Shipilev wrote:
> > > > 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
> > > > > > 
> > > > > > 
> > > > 
> > > 
> > 
> 
>