Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:54:16 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Address comments >> - Merge branch 'master' into 8329332 >> - Removed not_used state of nmethod >> - remove trailing whitespace >> - 8329332: Remove CompiledMethod and CodeBlobLayout classes > > I took a second pass over the changes. I've given a few suggestions below. > None of them should require respinning of tests (except for making sure that > this still builds). Thank you, @stefank , for second round of review. I addressed all your comments. I also removed `virtual` from `virtual method() override;` as you suggested before since I touched these files again. I will wait result of new GHA and tier1 in mach5 before integration. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2037731476
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 16:03:12 GMT, Stefan Karlsson wrote: >> Quote: "an" goes before words that begin with vowels. > > I don't think that holds if the 'n' is pronounced the way nmethod is > pronounced. `grep` shows that we have both cases but `an nmethod` is used more. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552024067
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 16:16:41 GMT, Vladimir Kozlov wrote: >> I don't think that holds if the 'n' is pronounced the way nmethod is >> pronounced. > > `grep` shows that we have both cases but `an nmethod` is used more. I will fix it here as you suggested but I am not touching other places. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552025626
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:51:47 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Address comments
>> - Merge branch 'master' into 8329332
>> - Removed not_used state of nmethod
>> - remove trailing whitespace
>> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
>
> src/hotspot/share/code/codeBlob.hpp line 168:
>
>> 166: bool is_vtable_blob() const { return _kind ==
>> CodeBlobKind::Blob_Vtable; }
>> 167: bool is_method_handles_adapter_blob() const { return _kind ==
>> CodeBlobKind::Blob_MH_Adapter; }
>> 168: bool is_upcall_stub() const { return _kind ==
>> CodeBlobKind::Blob_Upcall; }
>
> The `Blob_` prefix is now redundant since we always have to prefix with
> CodeBlobKind::. Just a suggestion if you want to shorten these.
Good suggestion
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552009581
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 15:56:34 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/gc/shared/gcBehaviours.hpp line 31: >> >>> 29: #include "oops/oopsHierarchy.hpp" >>> 30: >>> 31: // This is the behaviour for checking if a nmethod is unloading >> >> Maybe this should be *an* nmethod? > > Quote: "an" goes before words that begin with vowels. I don't think that holds if the 'n' is pronounced the way nmethod is pronounced. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1552005187
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:31:24 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Address comments
>> - Merge branch 'master' into 8329332
>> - Removed not_used state of nmethod
>> - remove trailing whitespace
>> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
>
> src/hotspot/share/gc/x/xUnload.cpp line 78:
>
>> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
>> 77: public:
>> 78: virtual bool has_dead_oop(nmethod* const nm) const {
>
> `nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
> variables, but not for variables in the parameter list). The same applies to
> the rest of the changes to this file.
Okay. I did not know that it is only used for locals. I will update code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551997411
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 07:26:21 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Address comments >> - Merge branch 'master' into 8329332 >> - Removed not_used state of nmethod >> - remove trailing whitespace >> - 8329332: Remove CompiledMethod and CodeBlobLayout classes > > src/hotspot/share/gc/shared/gcBehaviours.hpp line 31: > >> 29: #include "oops/oopsHierarchy.hpp" >> 30: >> 31: // This is the behaviour for checking if a nmethod is unloading > > Maybe this should be *an* nmethod? Quote: "an" goes before words that begin with vowels. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551989120
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
>> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
>> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
>> implementation in JDK 9. The code was left in HotSpot assuming it will help
>> in a future. But during work on Leyden we decided to not use it. In Leyden
>> cached compiled code will be restored in CodeCache as normal nmethods: no
>> need to change VM's runtime and GC code to process them.
>>
>> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
>> header size in separate changes. In these changes I did simple fields
>> reordering to keep small (1 byte) fields together.
>>
>> I do not see (and not expected) performance difference with these changes.
>>
>> Tested tier1-5, xcomp, stress. Running performance testing.
>>
>> I need help with testing on platforms which Oracle does not support.
>
> Vladimir Kozlov has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Address comments
> - Merge branch 'master' into 8329332
> - Removed not_used state of nmethod
> - remove trailing whitespace
> - 8329332: Remove CompiledMethod and CodeBlobLayout classes
I took a second pass over the changes. I've given a few suggestions below. None
of them should require respinning of tests (except for making sure that this
still builds).
src/hotspot/share/code/codeBlob.hpp line 168:
> 166: bool is_vtable_blob() const { return _kind ==
> CodeBlobKind::Blob_Vtable; }
> 167: bool is_method_handles_adapter_blob() const { return _kind ==
> CodeBlobKind::Blob_MH_Adapter; }
> 168: bool is_upcall_stub() const { return _kind ==
> CodeBlobKind::Blob_Upcall; }
The `Blob_` prefix is now redundant since we always have to prefix with
CodeBlobKind::. Just a suggestion if you want to shorten these.
src/hotspot/share/gc/shared/gcBehaviours.hpp line 31:
> 29: #include "oops/oopsHierarchy.hpp"
> 30:
> 31: // This is the behaviour for checking if a nmethod is unloading
Maybe this should be *an* nmethod?
src/hotspot/share/gc/shenandoah/shenandoahUnload.cpp line 81:
> 79: class ShenandoahIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 80: public:
> 81: virtual bool has_dead_oop(nmethod* const nm) const {
Is there a reason why this was changed to `nmethod* const nm` instead of
`nmethod* nm`? IsUnloadingBehviour::has_dead_oop uses `nmethod* nm`. This
question applies to the other changes in this file as well.
src/hotspot/share/gc/x/xUnload.cpp line 78:
> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 77: public:
> 78: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/gc/z/zUnload.cpp line 77:
> 75: class ZIsUnloadingBehaviour : public IsUnloadingBehaviour {
> 76: public:
> 77: virtual bool has_dead_oop(nmethod* const nm) const {
`nmethod* const nm` => `nmethod* nm`. (ZGC's style is to use const for local
variables, but not for variables in the parameter list). The same applies to
the rest of the changes to this file.
src/hotspot/share/runtime/javaThread.hpp line 123:
> 121: DeoptResourceMark* _deopt_mark; // Holds special
> ResourceMark for deoptimization
> 122:
> 123: nmethod* _deopt_nmethod; // nmethod that is
> currently being deoptimized
The alignment is (and was) weird here.
-
Marked as reviewed by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1978954058
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551107567
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551073461
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551077362
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080101
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551080280
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1551100400
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Address comments > - Merge branch 'master' into 8329332 > - Removed not_used state of nmethod > - remove trailing whitespace > - 8329332: Remove CompiledMethod and CodeBlobLayout classes There is a stale comment in `test/jdk/com/sun/jdi/EATests.java:1288` -// (See CompiledMethod::is_at_poll_return()) +// (See nmethod::is_at_poll_return()) - PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1978884423
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Address comments > - Merge branch 'master' into 8329332 > - Removed not_used state of nmethod > - remove trailing whitespace > - 8329332: Remove CompiledMethod and CodeBlobLayout classes GHA `linux-x64-hs-minimal` failure is not related to changes: 2024-04-04T00:07:46.9654262Z ##[warning]Failed to download action 'https://api.github.com/repos/actions/github-script/tarball/60a0d83039c74a4aee543508d2ffcb1c3799cdea'. Error: The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing. 2024-04-04T00:07:46.9656929Z ##[warning]Back off 22.252 seconds before retry. 2024-04-04T00:08:52.1252710Z ##[error]The SSL connection could not be established, see inner exception. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2035859221
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE > [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) > which was used for AOT [JEP 295](https://openjdk.org/jeps/295) > implementation in JDK 9. The code was left in HotSpot assuming it will help > in a future. But during work on Leyden we decided to not use it. In Leyden > cached compiled code will be restored in CodeCache as normal nmethods: no > need to change VM's runtime and GC code to process them. > > I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce > header size in separate changes. In these changes I did simple fields > reordering to keep small (1 byte) fields together. > > I do not see (and not expected) performance difference with these changes. > > Tested tier1-5, xcomp, stress. Running performance testing. > > I need help with testing on platforms which Oracle does not support. Vladimir Kozlov has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - Address comments - Merge branch 'master' into 8329332 - Removed not_used state of nmethod - remove trailing whitespace - 8329332: Remove CompiledMethod and CodeBlobLayout classes - Changes: - all: https://git.openjdk.org/jdk/pull/18554/files - new: https://git.openjdk.org/jdk/pull/18554/files/246ff68a..33768fb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18554=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18554=01-02 Stats: 9283 lines in 197 files changed: 3058 ins; 4514 del; 1711 mod Patch: https://git.openjdk.org/jdk/pull/18554.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18554/head:pull/18554 PR: https://git.openjdk.org/jdk/pull/18554
