RFR: 8261868: Reduce inclusion of metaspace.hpp

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

RFR: 8261868: Reduce inclusion of metaspace.hpp

Ioi Lam-2
metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.

- classLoaderData.hpp doesn't actually need metaspace.hpp.
- array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp

Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).

Also, these 3 includes can now be removed from metaspace.hpp.

#include "memory/memRegion.hpp"
#include "memory/metaspaceChunkFreeListSummary.hpp"
#include "memory/virtualspace.hpp"

Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.

(I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).

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

Commit messages:
 - step2
 - step 1

Changes: https://git.openjdk.java.net/jdk/pull/2599/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2599&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261868
  Stats: 361 lines in 49 files changed: 221 ins; 132 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2599.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2599/head:pull/2599

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Ioi Lam-2
> metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.
>
> - classLoaderData.hpp doesn't actually need metaspace.hpp.
> - array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp
>
> Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).
>
> Also, these 3 includes can now be removed from metaspace.hpp.
>
> #include "memory/memRegion.hpp"
> #include "memory/metaspaceChunkFreeListSummary.hpp"
> #include "memory/virtualspace.hpp"
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>
> (I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

  fixed ppc/s390 builds

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2599/files
  - new: https://git.openjdk.java.net/jdk/pull/2599/files/7a32ad1d..700d1c16

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

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

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Thomas Stuefe
On Wed, 17 Feb 2021 07:31:12 GMT, Ioi Lam <[hidden email]> wrote:

>> metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.
>>
>> - classLoaderData.hpp doesn't actually need metaspace.hpp.
>> - array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp
>>
>> Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).
>>
>> Also, these 3 includes can now be removed from metaspace.hpp.
>>
>> #include "memory/memRegion.hpp"
>> #include "memory/metaspaceChunkFreeListSummary.hpp"
>> #include "memory/virtualspace.hpp"
>>
>> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>>
>> (I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   fixed ppc/s390 builds

Hi Ioi,

this is very appreciated.

metaspace.hpp is still a bit of a mess. Its the last holdover for the old metaspace implementation and I always wanted to clean it out a bit. Splitting this header into three is a right step.

A lot of that stuff may still vanish and/or be reformed if I have the time (eg metaspaceChunkFreeListSummary).

Assuming this builds and tests fine on all our platform, including the weirder ones, I am fine with this patch. It looks good.

Thanks, Thomas

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

Marked as reviewed by stuefe (Reviewer).

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Calvin Cheung
In reply to this post by Ioi Lam-2
On Wed, 17 Feb 2021 07:31:12 GMT, Ioi Lam <[hidden email]> wrote:

>> metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.
>>
>> - classLoaderData.hpp doesn't actually need metaspace.hpp.
>> - array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp
>>
>> Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).
>>
>> Also, these 3 includes can now be removed from metaspace.hpp.
>>
>> #include "memory/memRegion.hpp"
>> #include "memory/metaspaceChunkFreeListSummary.hpp"
>> #include "memory/virtualspace.hpp"
>>
>> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>>
>> (I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   fixed ppc/s390 builds

One more file you may consider updating is share/memory/classLoaderMetaspace.cpp.
It now depends on metaspaceUtils.hpp but it includes it transitively via metaspaceTracer.hpp.
The include of metaspace.hpp is not needed because classLoaderMetaspace.hpp includes it.

Other changes seem good.

Thanks,
Calvin

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

Marked as reviewed by ccheung (Reviewer).

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.
>
> - classLoaderData.hpp doesn't actually need metaspace.hpp.
> - array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp
>
> Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).
>
> Also, these 3 includes can now be removed from metaspace.hpp.
>
> #include "memory/memRegion.hpp"
> #include "memory/metaspaceChunkFreeListSummary.hpp"
> #include "memory/virtualspace.hpp"
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>
> (I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).

Ioi Lam 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 six additional commits since the last revision:

 - fixed copyright
 - Merge branch 'master' of https://github.com/openjdk/jdk into 8261868-reduce-inclusion-of-metaspace.hpp
 - @calvinccheung review comments
 - fixed ppc/s390 builds
 - step2
 - step 1

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2599/files
  - new: https://git.openjdk.java.net/jdk/pull/2599/files/700d1c16..b5b5a935

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

  Stats: 15191 lines in 386 files changed: 10589 ins; 2402 del; 2200 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2599.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2599/head:pull/2599

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Ioi Lam-2
In reply to this post by Thomas Stuefe
On Wed, 17 Feb 2021 07:54:28 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   fixed ppc/s390 builds
>
> Hi Ioi,
>
> this is very appreciated.
>
> metaspace.hpp is still a bit of a mess. Its the last holdover for the old metaspace implementation and I always wanted to clean it out a bit. Splitting this header into three is a right step.
>
> A lot of that stuff may still vanish and/or be reformed if I have the time (eg metaspaceChunkFreeListSummary).
>
> Assuming this builds and tests fine on all our platform, including the weirder ones, I am fine with this patch. It looks good.
>
> Thanks, Thomas

Thanks @tstuefe and @calvinccheung for the review. I re-tested with the latest repo.

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

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Ioi Lam-2
In reply to this post by Calvin Cheung
On Fri, 19 Feb 2021 06:08:09 GMT, Calvin Cheung <[hidden email]> wrote:

> One more file you may consider updating is share/memory/classLoaderMetaspace.cpp.
> It now depends on metaspaceUtils.hpp but it includes it transitively via metaspaceTracer.hpp.
> The include of metaspace.hpp is not needed because classLoaderMetaspace.hpp includes it.
>
> Other changes seem good.

I added metaspaceUtils.hpp to classLoaderMetaspace.cpp. I kept metaspace.hpp in there -- our convention is to always explicitly include a header file if we use its contents, even if this header file might be transitively included by other headers.

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

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

Withdrawn: 8261868: Reduce inclusion of metaspace.hpp

duke
In reply to this post by Ioi Lam-2
On Wed, 17 Feb 2021 06:12:37 GMT, Ioi Lam <[hidden email]> wrote:

> metaspace.hpp is included by about 770 out of 1000 HotSpot .o files. Most of these are transitively included via array.hpp and classLoaderData.hpp.
>
> - classLoaderData.hpp doesn't actually need metaspace.hpp.
> - array.hpp can be refactored to put a function that depends on metaspace.hpp into array.inline.hpp
>
> Doing the above reduces the number of .o files that include metaspace.hpp to 343. Since this is still a significant number, we should split out the rarely used classes (such as `MetaspaceGC` and `MetaspaceUtils`) into a new header file (metaspaceUtils.hpp, which is included only 30 times).
>
> Also, these 3 includes can now be removed from metaspace.hpp.
>
> #include "memory/memRegion.hpp"
> #include "memory/metaspaceChunkFreeListSummary.hpp"
> #include "memory/virtualspace.hpp"
>
> Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
>
> (I also fixed an unrelated comment in archiveUtils.cpp when I was scanning for the word "Metaspace" in the source files -- the function `MetaspaceShared::commit_to()` no longer exists).

This pull request has been closed without being integrated.

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

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

Re: RFR: 8261868: Reduce inclusion of metaspace.hpp [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Thu, 25 Feb 2021 04:30:42 GMT, Ioi Lam <[hidden email]> wrote:

>> Hi Ioi,
>>
>> this is very appreciated.
>>
>> metaspace.hpp is still a bit of a mess. Its the last holdover for the old metaspace implementation and I always wanted to clean it out a bit. Splitting this header into three is a right step.
>>
>> A lot of that stuff may still vanish and/or be reformed if I have the time (eg metaspaceChunkFreeListSummary).
>>
>> Assuming this builds and tests fine on all our platform, including the weirder ones, I am fine with this patch. It looks good.
>>
>> Thanks, Thomas
>
> Thanks @tstuefe and @calvinccheung for the review. I re-tested with the latest repo.

Note: the commit into the openjdk/jdk repo was successful. https://github.com/openjdk/jdk/commit/0f8be6e433b5d30e028558a4bea0659838d6b700

The previous message by the openjdk bot seems to be an error by the bot.

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

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