RFR: 8264731: Introduce InstanceKlass::method_at_itable_or_null()

[Vladimir Ivanov-2]

Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.

As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.

Testing:
* [x] hs-tier1 - hs-tier6

-------------

Commit messages:
 - InstanceKlass::method_at_itable_or_null

Changes: https://git.openjdk.java.net/jdk/pull/3344/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3344&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264731
  Stats: 46 lines in 3 files changed: 14 ins; 9 del; 23 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3344.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3344/head:pull/3344

PR: https://git.openjdk.java.net/jdk/pull/3344

[Coleen Phillimore-3]

On Mon, 5 Apr 2021 17:40:58 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>
> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

This is a lot clearer than existing code.  I don't see the caller other than mehod_at_itable() though.

src/hotspot/share/oops/instanceKlass.cpp line 3179:

> 3177:
> 3178: Method* InstanceKlass::method_at_itable_or_null(InstanceKlass* holder, int index, bool& itable_entry_found) {
> 3179:   klassItable itable(this);

I think  you need a ResourceMark before this.

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3344

[David Holmes-2]

On Mon, 5 Apr 2021 17:40:58 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>
> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

Hi Vladimir,

Seems okay in principle. I assume more callers of the new API are in the pipeline?

I have one suggested changed that would have made the code much much clearer to me.

Thanks,
David

src/hotspot/share/oops/instanceKlass.cpp line 3154:

> 3152:
> 3153: Method* InstanceKlass::method_at_itable(InstanceKlass* holder, int index, TRAPS) {
> 3154:   bool itable_entry_found; // out parameter

I was very confused about the logic in this code until I realized that itable_entry_found is actually an indicator as to whether or not the current class implements the interface represented by holder, and not an indicator of whether or not a method was found. It would be much clearer to me if this variable were renamed something like implements_interface.

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>
> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:

  Cleanups

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3344/files
  - new: https://git.openjdk.java.net/jdk/pull/3344/files/2755b483..91b977c0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3344&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3344&range=00-01

  Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3344.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3344/head:pull/3344

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

On Tue, 6 Apr 2021 02:52:50 GMT, David Holmes <[hidden email]> wrote:

> I assume more callers of the new API are in the pipeline?

Yes, the plan is to use it in CHA (`dependencies.cpp/hpp`). I decided to integrate runtime-related code changes separately.

> src/hotspot/share/oops/instanceKlass.cpp line 3154:
>
>> 3152:
>> 3153: Method* InstanceKlass::method_at_itable(InstanceKlass* holder, int index, TRAPS) {
>> 3154:   bool itable_entry_found; // out parameter
>
> I was very confused about the logic in this code until I realized that itable_entry_found is actually an indicator as to whether or not the current class implements the interface represented by holder, and not an indicator of whether or not a method was found. It would be much clearer to me if this variable were renamed something like implements_interface.

Agree. Fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

On Tue, 6 Apr 2021 02:01:29 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Cleanups
>
> src/hotspot/share/oops/instanceKlass.cpp line 3179:
>
>> 3177:
>> 3178: Method* InstanceKlass::method_at_itable_or_null(InstanceKlass* holder, int index, bool& itable_entry_found) {
>> 3179:   klassItable itable(this);
>
> I think  you need a ResourceMark before this.

Can you elaborate, please, why you think so?
I don't see anything in `klassItable` which may allocate in the resource arena.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3344

[Coleen Phillimore-3]

On Tue, 6 Apr 2021 13:02:38 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> src/hotspot/share/oops/instanceKlass.cpp line 3179:
>>
>>> 3177:
>>> 3178: Method* InstanceKlass::method_at_itable_or_null(InstanceKlass* holder, int index, bool& itable_entry_found) {
>>> 3179:   klassItable itable(this);
>>
>> I think  you need a ResourceMark before this.
>
> Can you elaborate, please, why you think so?
> I don't see anything in `klassItable` which may allocate in the resource arena.

oh, never mind. Somebody fixed this a long time ago.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3344

[David Holmes-2]

On Tue, 6 Apr 2021 13:01:53 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>>
>> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Cleanups

Update looks good - thanks - but I'd like to see it extend a little further.

Thanks,
David

src/hotspot/share/oops/instanceKlass.cpp line 3180:

> 3178: }
> 3179:
> 3180: Method* InstanceKlass::method_at_itable_or_null(InstanceKlass* holder, int index, bool& itable_entry_found) {

The change to use implements_interface applies in here too. Thanks.

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>
> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:

  itable_entry_found -> implements_interface renaming

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3344/files
  - new: https://git.openjdk.java.net/jdk/pull/3344/files/91b977c0..df179a08

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3344&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3344&range=01-02

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3344.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3344/head:pull/3344

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

On Wed, 7 Apr 2021 05:26:01 GMT, David Holmes <[hidden email]> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Cleanups
>
> Update looks good - thanks - but I'd like to see it extend a little further.
>
> Thanks,
> David

Thanks for the reviews, Coleen and David.

> src/hotspot/share/oops/instanceKlass.cpp line 3180:
>
>> 3178: }
>> 3179:
>> 3180: Method* InstanceKlass::method_at_itable_or_null(InstanceKlass* holder, int index, bool& itable_entry_found) {
>
> The change to use implements_interface applies in here too. Thanks.

Sure, fixed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3344

[Vladimir Ivanov-2]

On Mon, 5 Apr 2021 17:40:58 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Introduce `InstanceKlass::method_at_itable_or_null()` - a non-throwing variant of `InstanceKlass::method_at_itable()` that implements interface method selection.
>
> As a cleanup, rewrite `InstanceKlass::method_at_itable()` on top of `InstanceKlass::method_at_itable_or_null()`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

This pull request has now been integrated.

Changeset: 6e2b82a4
Author:    Vladimir Ivanov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/6e2b82a4
Stats:     48 lines in 3 files changed: 16 ins; 9 del; 23 mod

8264731: Introduce InstanceKlass::method_at_itable_or_null()

Reviewed-by: coleenp, dholmes

-------------

PR: https://git.openjdk.java.net/jdk/pull/3344