RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Vladimir Ivanov-2
Turn `resolved_method` parameter into raw `Method*`.

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

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

Commit messages:
 - LinkResolver::vtable_index_of_interface_method

Changes: https://git.openjdk.java.net/jdk/pull/3346/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3346&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264732
  Stats: 7 lines in 3 files changed: 0 ins; 2 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3346/head:pull/3346

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Lois Foltan-3
On Mon, 5 Apr 2021 18:01:15 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

Looks good to me.
Lois

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

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Coleen Phillimore-3
In reply to this post by Vladimir Ivanov-2
On Mon, 5 Apr 2021 18:01:15 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 731:

> 729: C2V_VMENTRY_0(jint, getVtableIndexForInterfaceMethod, (JNIEnv* env, jobject, jobject jvmci_type, jobject jvmci_method))
> 730:   Klass* klass = JVMCIENV->asKlass(jvmci_type);
> 731:   Method* method = JVMCIENV->asMethod(jvmci_method);

The reason that resolved_method was a methodHandle is in case of redefinition, we need to know if code is referring to this version of the method so that it's not deallocated.   It's enough for one of the callers to create a methodHandle but passing the methodHandle will guarantee it.  I'm not sure why you needed to make this change.

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

Vladimir Ivanov-2
In reply to this post by Vladimir Ivanov-2
> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

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

  Revert jvmciCompilerToVM.cpp change.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3346/files
  - new: https://git.openjdk.java.net/jdk/pull/3346/files/89e0b67d..a2bf4794

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

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

Vladimir Ivanov-2
In reply to this post by Coleen Phillimore-3
On Mon, 5 Apr 2021 19:09:21 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert jvmciCompilerToVM.cpp change.
>
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 731:
>
>> 729: C2V_VMENTRY_0(jint, getVtableIndexForInterfaceMethod, (JNIEnv* env, jobject, jobject jvmci_type, jobject jvmci_method))
>> 730:   Klass* klass = JVMCIENV->asKlass(jvmci_type);
>> 731:   Method* method = JVMCIENV->asMethod(jvmci_method);
>
> The reason that resolved_method was a methodHandle is in case of redefinition, we need to know if code is referring to this version of the method so that it's not deallocated.   It's enough for one of the callers to create a methodHandle but passing the methodHandle will guarantee it.  I'm not sure why you needed to make this change.

I refactored `getVtableIndexForInterfaceMethod` and kept the handle in place while dereferencing it when passing into `vtable_index_of_interface_method`. Does it look better now?

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

Coleen Phillimore-3
In reply to this post by Vladimir Ivanov-2
On Tue, 6 Apr 2021 13:41:48 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Turn `resolved_method` parameter into raw `Method*`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revert jvmciCompilerToVM.cpp change.

Marked as reviewed by coleenp (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

Coleen Phillimore-3
In reply to this post by Vladimir Ivanov-2
On Tue, 6 Apr 2021 13:37:53 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 731:
>>
>>> 729: C2V_VMENTRY_0(jint, getVtableIndexForInterfaceMethod, (JNIEnv* env, jobject, jobject jvmci_type, jobject jvmci_method))
>>> 730:   Klass* klass = JVMCIENV->asKlass(jvmci_type);
>>> 731:   Method* method = JVMCIENV->asMethod(jvmci_method);
>>
>> The reason that resolved_method was a methodHandle is in case of redefinition, we need to know if code is referring to this version of the method so that it's not deallocated.   It's enough for one of the callers to create a methodHandle but passing the methodHandle will guarantee it.  I'm not sure why you needed to make this change.
>
> I refactored `getVtableIndexForInterfaceMethod` and kept the handle in place while dereferencing it when passing into `vtable_index_of_interface_method`. Does it look better now?

Ok, this is fine.  Be careful if you have new callers for this function.

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

David Holmes-2
In reply to this post by Vladimir Ivanov-2
On Tue, 6 Apr 2021 13:41:48 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Turn `resolved_method` parameter into raw `Method*`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Revert jvmciCompilerToVM.cpp change.

Seems quite reasonable - though unsure of the motivation.

Do we need a NoSafepointVerifier in LinkResolver::vtable_index_of_interface_method to ensure the raw Method* remains valid?

Thanks,
David

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v3]

Vladimir Ivanov-2
In reply to this post by Vladimir Ivanov-2
> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

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

  Move itable_index_of_interface_method() implementation to InstanceKlass

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3346/files
  - new: https://git.openjdk.java.net/jdk/pull/3346/files/a2bf4794..9dfc9437

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

  Stats: 55 lines in 5 files changed: 28 ins; 21 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3346/head:pull/3346

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v4]

Vladimir Ivanov-2
In reply to this post by Vladimir Ivanov-2
> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

Vladimir Ivanov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:

 - Merge branch 'master' into 8264732.vtable_index_of_interface_method
 - Move itable_index_of_interface_method() implementation to InstanceKlass
 - Revert jvmciCompilerToVM.cpp change.
 - LinkResolver::vtable_index_of_interface_method

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

Changes: https://git.openjdk.java.net/jdk/pull/3346/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3346&range=03
  Stats: 55 lines in 3 files changed: 29 ins; 23 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3346.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3346/head:pull/3346

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v2]

Vladimir Ivanov-2
In reply to this post by David Holmes-2
On Wed, 7 Apr 2021 05:32:46 GMT, David Holmes <[hidden email]> wrote:

>> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Revert jvmciCompilerToVM.cpp change.
>
> Seems quite reasonable - though unsure of the motivation.
>
> Do we need a NoSafepointVerifier in LinkResolver::vtable_index_of_interface_method to ensure the raw Method* remains valid?
>
> Thanks,
> David

Thinking more about it, `methodHandle` does look more natural than a raw `Method*` on `LinkResolver`.

What do you think about leaving `LinkResolver` as it is now and move `vtable_index_of_interface_method` implementation to `InstanceKlass` instead?

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v3]

Coleen Phillimore-3
In reply to this post by Vladimir Ivanov-2
On Wed, 7 Apr 2021 21:41:58 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Turn `resolved_method` parameter into raw `Method*`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Move itable_index_of_interface_method() implementation to InstanceKlass

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

> 3197:   // First check in default method array
> 3198:   if (!intf_method->is_abstract() && this->default_methods() != NULL) {
> 3199:     int index = InstanceKlass::find_method_index(this->default_methods(),

Yes, please, this looks a lot better!  Then linkResolver can deal with methodHandle consistently so it doesn't go away with redefinition, and this function is nice here with the other like functions.
It doesn't need InstanceKlass:: before find_method_index, or the this->'s.

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v4]

David Holmes-2
In reply to this post by Vladimir Ivanov-2
On Wed, 7 Apr 2021 21:43:48 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Turn `resolved_method` parameter into raw `Method*`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>
>  - Merge branch 'master' into 8264732.vtable_index_of_interface_method
>  - Move itable_index_of_interface_method() implementation to InstanceKlass
>  - Revert jvmciCompilerToVM.cpp change.
>  - LinkResolver::vtable_index_of_interface_method

Refactoring looks good.

I second Coleen's comment about unnecessary classname qualification and use of this->

Thanks,
David

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v5]

Vladimir Ivanov-2
In reply to this post by Vladimir Ivanov-2
> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

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

  InstanceKlass::method_at_itable_or_null() cleanups

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3346/files
  - new: https://git.openjdk.java.net/jdk/pull/3346/files/b6737c15..b5aaf41e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3346&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3346&range=03-04

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v4]

Vladimir Ivanov-2
In reply to this post by David Holmes-2
On Thu, 8 Apr 2021 10:14:51 GMT, David Holmes <[hidden email]> wrote:

>> Vladimir Ivanov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>>
>>  - Merge branch 'master' into 8264732.vtable_index_of_interface_method
>>  - Move itable_index_of_interface_method() implementation to InstanceKlass
>>  - Revert jvmciCompilerToVM.cpp change.
>>  - LinkResolver::vtable_index_of_interface_method
>
> Refactoring looks good.
>
> I second Coleen's comment about unnecessary classname qualification and use of this->
>
> Thanks,
> David

Thanks for the reviews, Lois, Coleen, and David.

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v3]

Vladimir Ivanov-2
In reply to this post by Coleen Phillimore-3
On Wed, 7 Apr 2021 23:00:46 GMT, Coleen Phillimore <[hidden email]> wrote:

> It doesn't need InstanceKlass:: before find_method_index, or the this->'s.

Fixed. It looked clearer to me to explicitly refer to `this`.

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264732: Clean up LinkResolver::vtable_index_of_interface_method() [v5]

David Holmes-2
In reply to this post by Vladimir Ivanov-2
On Thu, 8 Apr 2021 10:30:43 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Turn `resolved_method` parameter into raw `Method*`.
>>
>> Testing:
>> * [x] hs-tier1 - hs-tier6
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
>
>   InstanceKlass::method_at_itable_or_null() cleanups

Marked as reviewed by dholmes (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3346
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Vladimir Ivanov-2
In reply to this post by Vladimir Ivanov-2
On Mon, 5 Apr 2021 18:01:15 GMT, Vladimir Ivanov <[hidden email]> wrote:

> Turn `resolved_method` parameter into raw `Method*`.
>
> Testing:
> * [x] hs-tier1 - hs-tier6

This pull request has now been integrated.

Changeset: 33fa855d
Author:    Vladimir Ivanov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/33fa855d
Stats:     55 lines in 3 files changed: 29 ins; 23 del; 3 mod

8264732: Clean up LinkResolver::vtable_index_of_interface_method()

Reviewed-by: lfoltan, coleenp, dholmes

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

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