8265696: Move CDS sources to src/hotspot/shared/cds #3610

iklam · 2021-04-21T21:55:25Z

The number of CDS source files have grown significantly. To improve modularity, the following files should be moved a new directory, src/hotspot/share/cds.

src/hotspot/share/classfile/classListParser.cpp
src/hotspot/share/classfile/classListParser.hpp
src/hotspot/share/classfile/classListWriter.hpp
src/hotspot/share/classfile/compactHashtable.cpp
src/hotspot/share/classfile/compactHashtable.hpp
src/hotspot/share/classfile/lambdaFormInvokers.cpp
src/hotspot/share/classfile/lambdaFormInvokers.hpp
src/hotspot/share/memory/archiveBuilder.cpp
src/hotspot/share/memory/archiveBuilder.hpp
src/hotspot/share/memory/archiveUtils.cpp
src/hotspot/share/memory/archiveUtils.hpp
src/hotspot/share/memory/archiveUtils.inline.hpp
src/hotspot/share/memory/cppVtables.cpp
src/hotspot/share/memory/cppVtables.hpp
src/hotspot/share/memory/dumpAllocStats.cpp
src/hotspot/share/memory/dumpAllocStats.hpp
src/hotspot/share/memory/dynamicArchive.cpp
src/hotspot/share/memory/dynamicArchive.hpp
src/hotspot/share/memory/filemap.cpp
src/hotspot/share/memory/filemap.hpp
src/hotspot/share/memory/heapShared.cpp
src/hotspot/share/memory/heapShared.hpp
src/hotspot/share/memory/heapShared.inline.hpp
src/hotspot/share/memory/metaspaceShared.cpp
src/hotspot/share/memory/metaspaceShared.hpp
src/hotspot/share/prims/cdsoffsets.cpp
src/hotspot/share/prims/cdsoffsets.hpp

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8265696: Move CDS sources to src/hotspot/shared/cds

Reviewers

Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 729c651
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 5ca1c51
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to ba45831

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610
$ git checkout pull/3610

Update a local copy of the PR:
$ git checkout pull/3610
$ git pull https://git.openjdk.java.net/jdk pull/3610/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3610

View PR using the GUI difftool:
$ git pr show -t 3610

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3610.diff

bridgekeeper · 2021-04-21T21:56:18Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-04-21T21:57:59Z

@iklam The following labels will be automatically applied to this pull request:

build
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

iklam · 2021-04-21T22:03:09Z

/label remove serviceability

openjdk · 2021-04-21T22:04:07Z

@iklam
The serviceability label was successfully removed.

iklam · 2021-04-21T22:04:13Z

make/hotspot/lib/JvmFeatures.gmk

-      metaspaceShared_$(HOTSPOT_TARGET_CPU).cpp \
-      metaspaceShared_$(HOTSPOT_TARGET_CPU_ARCH).cpp \
-      sharedClassUtil.cpp \
-      sharedPathsMiscInfo.cpp \


Removed obsolete files that no longer exist.

I don't suppose we can just exclude the new directory rather than listing individual files?

Fixed. Now all files under share/cds are excluded. I needed to move compactHashtable.cpp back to its old location since a little of it is used even when CDS is not used.

That's indeed a better idea.

iklam · 2021-04-21T22:05:00Z

src/hotspot/os/windows/os_windows.cpp

@@ -39,7 +39,6 @@
 #include "logging/log.hpp"
 #include "logging/logStream.hpp"
 #include "memory/allocation.inline.hpp"
-#include "memory/filemap.hpp"
 #include "oops/oop.inline.hpp"


Removed some unnecessary inclusions.

mlbridge · 2021-04-21T22:57:50Z

Webrevs

erikj79

Build changes look good.

erikj79 · 2021-04-21T23:05:26Z

make/hotspot/lib/JvmFeatures.gmk

-      metaspaceShared_$(HOTSPOT_TARGET_CPU).cpp \
-      metaspaceShared_$(HOTSPOT_TARGET_CPU_ARCH).cpp \
-      sharedClassUtil.cpp \
-      sharedPathsMiscInfo.cpp \


dholmes-ora

Hi Ioi,

Moving these files to their own directory is fine, but all the moved headers need further adjustments for the include guards.

Thanks,
David

dholmes-ora · 2021-04-22T04:14:38Z

make/hotspot/lib/JvmFeatures.gmk

-      metaspaceShared_$(HOTSPOT_TARGET_CPU).cpp \
-      metaspaceShared_$(HOTSPOT_TARGET_CPU_ARCH).cpp \
-      sharedClassUtil.cpp \
-      sharedPathsMiscInfo.cpp \


I don't suppose we can just exclude the new directory rather than listing individual files?

dholmes-ora · 2021-04-22T04:16:57Z

src/hotspot/share/cds/archiveUtils.inline.hpp

@@ -25,7 +25,7 @@
 #ifndef SHARE_MEMORY_ARCHIVEUTILS_INLINE_HPP


The header file include guards all need updating for the new path.

dholmes-ora · 2021-04-22T04:18:16Z

src/hotspot/share/cds/dynamicArchive.hpp

 #include "memory/memRegion.hpp"
 #include "memory/virtualspace.hpp"
 #include "oops/oop.hpp"
 #include "utilities/exceptions.hpp"
 #include "utilities/macros.hpp"
 #include "utilities/resourceHash.hpp"

+#if INCLUDE_CDS


I have to wonder who is including this file and why, if CDS is not enabled.

E.g., jvm.cpp includes dynamicArchive.hpp, but only uses its declarations inside INCLUDE_CDS blocks. It would be too verbose to do this in jvm.cpp

#if INCLUDE_CDS #include "cds/dynamicArchive.hpp" #endif

We routinely do that for other INCLUDE_X features.

…le.cpp cannot be excluded since a bit of it is used even when CDS is disabled

tstuefe

Hi @iklam,

this is a very welcome change!

Nothing much to add to what David wrote (include guards need renaming).

Apart from that, I was surprised that no gtests needed to be adapted, but seems cds has no gtests?

I tested building without cds, works fine.

Thanks for doing this!

If you fix the include guards, this is fine by me.

..Thomas

iklam · 2021-04-22T06:20:46Z

Hi @iklam,

this is a very welcome change!

Nothing much to add to what David wrote (include guards need renaming).

Apart from that, I was surprised that no gtests needed to be adapted, but seems cds has no gtests?

I tested building without cds, works fine.

Thanks for doing this!

If you fix the include guards, this is fine by me.

..Thomas

Hi Thomas, thanks for the review. You're right that we don't have any gtests .... that should be fixed at some point.

dholmes-ora

Updates look good.

Thanks,
David

erikj79 · 2021-04-22T12:31:21Z

make/hotspot/lib/JvmFeatures.gmk

-      metaspaceShared_$(HOTSPOT_TARGET_CPU).cpp \
-      metaspaceShared_$(HOTSPOT_TARGET_CPU_ARCH).cpp \
-      sharedClassUtil.cpp \
-      sharedPathsMiscInfo.cpp \


That's indeed a better idea.

openjdk · 2021-04-22T15:49:41Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265696: Move CDS sources to src/hotspot/shared/cds

Reviewed-by: erikj, dholmes, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iklam · 2021-04-23T04:11:46Z

Thanks @tstuefe @erikj79 @dholmes-ora for the review.
The latest merged code passes Mach5 build tiers 1-5.
I also manually tested the minimal VM build to verify the exclusion of CDS object files.
/integrate

openjdk · 2021-04-23T04:13:26Z

@iklam Pushed as commit 95f0fd6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

iklam added 4 commits April 21, 2021 10:29

8265696: Move CDS sources from shared/memory to shared/cds

c120378

fixed include lines

061464c

moved more files

6fc1bd3

fixed copyright

ba45831

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org labels Apr 21, 2021

openjdk bot removed the serviceability serviceability-dev@openjdk.org label Apr 21, 2021

iklam commented Apr 21, 2021

View reviewed changes

iklam marked this pull request as ready for review April 21, 2021 22:53

openjdk bot added the rfr Pull request is ready for review label Apr 21, 2021

erikj79 approved these changes Apr 21, 2021

View reviewed changes

dholmes-ora suggested changes Apr 22, 2021

View reviewed changes

iklam added 3 commits April 21, 2021 22:01

fixed include guards -> #ifndef SHARE_CDS_xxxxx

e8a320e

exclude all files under shared/cds if CDS is disabled; compactHashtab…

a1a2699

…le.cpp cannot be excluded since a bit of it is used even when CDS is disabled

Merge branch 'master' into 8265696-move-cds-sources

729c651

tstuefe approved these changes Apr 22, 2021

View reviewed changes

dholmes-ora approved these changes Apr 22, 2021

View reviewed changes

erikj79 approved these changes Apr 22, 2021

View reviewed changes

iklam changed the title ~~8265696 move cds sources~~ 8265696: Move CDS sources to src/hotspot/shared/cds Apr 22, 2021

openjdk bot added the ready Pull request is ready to be integrated label Apr 22, 2021

iklam added 2 commits April 22, 2021 16:49

Merge branch 'master' into 8265696-move-cds-sources

43c75a7

Merge branch 'master' into 8265696-move-cds-sources

41b8b48

iklam added 2 commits April 22, 2021 17:18

fixed merge

5ca1c51

Merge branch 'master' into 8265696-move-cds-sources

8a9e7bd

dholmes-ora approved these changes Apr 23, 2021

View reviewed changes

openjdk bot closed this Apr 23, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8265696: Move CDS sources to src/hotspot/shared/cds #3610

8265696: Move CDS sources to src/hotspot/shared/cds #3610

iklam commented Apr 21, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

iklam commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

iklam Apr 21, 2021

erikj79 Apr 21, 2021

dholmes-ora Apr 22, 2021

iklam Apr 22, 2021

erikj79 Apr 22, 2021

iklam Apr 21, 2021

mlbridge bot commented Apr 21, 2021 •

edited

Loading

erikj79 left a comment

erikj79 Apr 21, 2021

dholmes-ora left a comment

dholmes-ora Apr 22, 2021

dholmes-ora Apr 22, 2021

iklam Apr 22, 2021

dholmes-ora Apr 22, 2021

iklam Apr 22, 2021

dholmes-ora Apr 22, 2021

tstuefe left a comment

iklam commented Apr 22, 2021

dholmes-ora left a comment

erikj79 Apr 22, 2021

openjdk bot commented Apr 22, 2021 •

edited

Loading

iklam commented Apr 23, 2021

openjdk bot commented Apr 23, 2021

		@@ -25,7 +25,7 @@
		#ifndef SHARE_MEMORY_ARCHIVEUTILS_INLINE_HPP

8265696: Move CDS sources to src/hotspot/shared/cds #3610

8265696: Move CDS sources to src/hotspot/shared/cds #3610

Conversation

iklam commented Apr 21, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

iklam commented Apr 21, 2021

openjdk bot commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Apr 21, 2021 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

iklam commented Apr 22, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 22, 2021 • edited Loading

iklam commented Apr 23, 2021

openjdk bot commented Apr 23, 2021

iklam commented Apr 21, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 21, 2021 •

edited

Loading

openjdk bot commented Apr 22, 2021 •

edited

Loading