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 |
> 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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
Free forum by Nabble | Edit this page |