8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass #2301

iklam · 2021-01-28T21:00:04Z

This is the second step of #2246 (8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp). These are mostly boiler-plate changes done by scripts.

[1] Change calls like

SystemDictionary::Object_klass()
SystemDictionary::Throwable_klass_is_loaded()
SystemDictionary::box_klass_type()

to

vmClasses::Object_klass()
vmClasses::Throwable_klass_is_loaded()
vmClasses::box_klass_type()

[2] Remove unnecessary inclusion of systemDictionary.hpp (replace with vmClasses.hpp if necessary). In some cases, I have to add signature.hpp to some files, which only indirectly included signature.hpp through systemDictionary.hpp.

[3] In the previous PR, I incorrectly used the enum name VMClassID. This PR changes it to vmClassID to match the existing use of vmSymbolID and vmIntrinsicID.

Due to the refactoring of these two PRs, the number of HotSpot .o files that include systemDictionary.hpp decreases from 491 to 91. HotSpot build time is reduced by about 2%

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

Review Notes: if you don't want to scroll through 185 files, you may want to try:

curl https://github.com/openjdk/jdk/compare/1de3c554477497d1ceee573180940e8d38c364ee...e2f77252c8b3edd4d0071cfc014290568a16de9d.diff | \
  grep -v '^[+-][+-][+-]' | grep '^[+-]'

Progress

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

Issue

JDK-8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass

Reviewers

Lois Foltan (@lfoltan - Reviewer) ⚠️ Review applies to e1a0941
Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to 8df077d
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 7489267
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 7489267

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2301/head:pull/2301
$ git checkout pull/2301

…_klass()

bridgekeeper · 2021-01-28T21:00:55Z

👋 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-01-28T21:01:43Z

@iklam this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8260471-SystemDictionary-to-vmClasses-rename
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2021-01-28T21:02:19Z

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

hotspot
serviceability
shenandoah

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-01-28T21:02:36Z

To make reviewing the 185 files a little easier, I haven't changed the copyright dates yet. I'll do that before the final integration.

iklam · 2021-01-28T21:03:03Z

/label remove serviceability

openjdk · 2021-01-28T21:03:28Z

@iklam
The serviceability label was successfully removed.

mlbridge · 2021-01-28T21:07:56Z

Webrevs

lfoltan

Looks good!!
Lois

tstuefe · 2021-02-01T18:10:02Z

Hi Ioi,

Sorry, I still get build errors with this (linux x64, fastdebug, nopch):

/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/aot/aotCodeHeap.cpp: In member function 'void AOTCodeHeap::link_known_klasses()':
/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/aot/aotCodeHeap.cpp:397:32: error: 'Reference_klass' is not a member of 'SystemDictionary'
   link_klass(SystemDictionary::Reference_klass());
                                ^~~~~~~~~~~~~~~

Last Commit:

commit e1a09411c12cdd95bf1f8896100284e0697fafb7 (HEAD -> pull/2301)                                                                                                   │
Author: iklam <ilam3388@gmail.com>                                                                                                                                    │
Date:   Fri Jan 29 06:22:52 2021 -0800                                                                                                                                │
                                                                                                                                                                      │
    added missing #include systemDictionary.hpp

I'm also confused why no GA ran for this pr. I gave the patch a cursory read, it reads all okay but of course I cannot see from reading the patch whether we could miss some includes. I'd like to see the GA builds being successful, in this case also for the side platforms and minimal builds/zero.

Thanks, Thomas

iklam · 2021-02-01T19:12:16Z

Hi Ioi,

Sorry, I still get build errors with this (linux x64, fastdebug, nopch):

/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/aot/aotCodeHeap.cpp: In member function 'void AOTCodeHeap::link_known_klasses()':
/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/aot/aotCodeHeap.cpp:397:32: error: 'Reference_klass' is not a member of 'SystemDictionary'
   link_klass(SystemDictionary::Reference_klass());
                                ^~~~~~~~~~~~~~~

Last Commit:

commit e1a09411c12cdd95bf1f8896100284e0697fafb7 (HEAD -> pull/2301)                                                                                                   │
Author: iklam <ilam3388@gmail.com>                                                                                                                                    │
Date:   Fri Jan 29 06:22:52 2021 -0800                                                                                                                                │
                                                                                                                                                                      │
    added missing #include systemDictionary.hpp

I'm also confused why no GA ran for this pr. I gave the patch a cursory read, it reads all okay but of course I cannot see from reading the patch whether we could miss some includes. I'd like to see the GA builds being successful, in this case also for the side platforms and minimal builds/zero.

Thanks, Thomas

I disabled the GitHub Actions for my repo because they were creating too much noise. Instead, I've been testing my builds with Mach5, which has a much larger variety of builds. Unfortunately AOT is disabled by default in Mach5 (due to JDK-8255616: Removal of experimental features AOT and Graal JIT).

Now I've added AOT to my local builds to make sure I don't break it unintentionally. I am also re-enabling GitHub actions on my repo.

I verified that AOT builds again with 8df077d

hseigel

Changes look good!
Thanks, Harold

openjdk · 2021-02-01T20:31:26Z

@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:

8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass

Reviewed-by: lfoltan, hseigel, 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.

dholmes-ora

LGTM!

Thanks,
David

tstuefe · 2021-02-02T07:50:25Z

Looks good to me too. Local build worked for me now. I see GA builds went through too. Thanks for this work! Anything giving us faster builds is appreciated, since build time keeps creeping up.

..Thomas

iklam · 2021-02-02T17:19:00Z

Thanks @tstuefe, @hseigel, @dholmes-ora and @lfoltan for the review. I will merge/test/push tonight when the repo is relatively quiet.

iklam · 2021-02-02T23:40:04Z

/integrate

openjdk · 2021-02-02T23:40:50Z

@iklam Since your change was applied there have been 6 commits pushed to the master branch:

9af3339: 8261003: Bad Copyright header format after JDK-8183372
6dc3c6d: 8183372: Refactor java/lang/Class shell tests to java
105d3e8: 8260861: TrustStoreDescriptor log the same value
69189f8: 8256421: Add 2 HARICA roots to cacerts truststore
f546fd0: 8260902: CDS mapping errors should not lead to unconditional output
d7b1fc5: 8260707: java/lang/instrument/PremainClass/InheritAgent0100.java times out

Your commit was automatically rebased without conflicts.

Pushed as commit ffbcf1b.

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

8260471: Change SystemDictionary::xxx_klass() calls to vmClasses::xxx…

e2f7725

…_klass()

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jan 28, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Jan 28, 2021

openjdk bot removed the serviceability serviceability-dev@openjdk.org label Jan 28, 2021

iklam marked this pull request as ready for review January 28, 2021 21:04

openjdk bot added the rfr Pull request is ready for review label Jan 28, 2021

added missing #include systemDictionary.hpp

e1a0941

lfoltan approved these changes Feb 1, 2021

View reviewed changes

fixed AOT build

8df077d

Merge branch 'master' into 8260471-SystemDictionary-to-vmClasses-rename

22afceb

hseigel approved these changes Feb 1, 2021

View reviewed changes

fixed comments

533615b

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Feb 1, 2021

fixed SA (serviceability/sa/CDSJMapClstats.java)

7489267

dholmes-ora approved these changes Feb 2, 2021

View reviewed changes

tstuefe approved these changes Feb 2, 2021

View reviewed changes

iklam added 2 commits February 2, 2021 08:41

Merge branch 'master' into 8260471-SystemDictionary-to-vmClasses-rename

93d127e

updated copyright

7cffd43

openjdk bot closed this Feb 2, 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 Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass #2301

8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass #2301

iklam commented Jan 28, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

iklam commented Jan 28, 2021

iklam commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

mlbridge bot commented Jan 28, 2021 •

edited

Loading

lfoltan left a comment

tstuefe commented Feb 1, 2021

iklam commented Feb 1, 2021

hseigel left a comment

openjdk bot commented Feb 1, 2021 •

edited

Loading

dholmes-ora left a comment

tstuefe commented Feb 2, 2021

iklam commented Feb 2, 2021

iklam commented Feb 2, 2021

openjdk bot commented Feb 2, 2021

8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass #2301

8260471: Change SystemDictionary::X_klass calls to vmClasses::X_klass #2301

Conversation

iklam commented Jan 28, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

iklam commented Jan 28, 2021

iklam commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

mlbridge bot commented Jan 28, 2021 • edited Loading

Webrevs

lfoltan left a comment

Choose a reason for hiding this comment

tstuefe commented Feb 1, 2021

iklam commented Feb 1, 2021

hseigel left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 1, 2021 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

tstuefe commented Feb 2, 2021

iklam commented Feb 2, 2021

iklam commented Feb 2, 2021

openjdk bot commented Feb 2, 2021

iklam commented Jan 28, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 28, 2021 •

edited

Loading

openjdk bot commented Feb 1, 2021 •

edited

Loading