8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp #2246

iklam · 2021-01-26T23:13:33Z

The "well-known classes" API in systemDictionary.hpp is for accessing a fixed set of classes defined by the bootstrap loader. The rest of systemDictionary.hpp is for finding/defining arbitrary classes in arbitrary class loaders. For modularity, we should separate these two sets of APIs, and make them independent of each other.

The proposed API follows the existing pattern of accessing known symbols/intrinsics in vmSymbols.hpp and vmIntrinsics.hpp:

#include "classfile/vmClasses.hpp"
#include "classfile/vmIntrinsics.hpp"
#include "classfile/vmSymbols.hpp" // NEW

Symbol* s = vmSymbols::java_lang_Object();
vmIntrinsics::ID id = vmIntrinsics::_linkToSpecial;
Klass* k = vmClasses::Object_klass(); // NEW

Note: to help the review process and limit the amount of boiler-place changes, within this RFE, we do not change existing calls like SystemDictionary::Object_klass() to vmClasses::Object_klass(). That will be done in a subtask (JDK-8260471 -- preliminary webrev).

Within this RFE, we temporarily subclass SystemDictionary from vmClasses, so SystemDictionary::Object_klass() will continue to work.

Tested 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-8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 3d38039
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 3d38039

Download

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

…tem-dictionary

bridgekeeper · 2021-01-26T23:13:47Z

👋 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-26T23:14:43Z

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

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-01-26T23:46:23Z

/label remove serviceability

openjdk · 2021-01-26T23:46:50Z

@iklam
The serviceability label was successfully removed.

mlbridge · 2021-01-26T23:51:48Z

Webrevs

01: Full - Incremental (e585ddc)
00: Full (3d38039)

iklam · 2021-01-27T00:02:16Z

The name vmClasses is in lowerCamelCase, which is a deviation from the rest of the HotSpot code, where most classes have UpperCamelCase names. However, vmClasses and vmSymbols are often used together, like the following, so a consistent look would be nice:

JavaCalls::call_static(&result,
   vmClasses::MethodHandleNatives_klass(),
   vmSymbols::linkMethodHandleConstant_name(),
   vmSymbols::linkMethodHandleConstant_signature(),
   &args, CHECK_(empty));

mlbridge · 2021-01-27T00:57:47Z

Mailing list message from Coleen Phillimore on hotspot-dev:

On 1/26/21 7:04 PM, Ioi Lam wrote:

On Tue, 26 Jan 2021 23:13:33 GMT, Ioi Lam <iklam at openjdk.org> wrote:

The "well-known classes" API in [systemDictionary.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/systemDictionary.hpp#L103) is for accessing a fixed set of classes defined by the bootstrap loader. The rest of systemDictionary.hpp is for finding/defining arbitrary classes in arbitrary class loaders. For modularity, we should separate these two sets of APIs, and make them independent of each other.

The proposed API follows the existing pattern of accessing known symbols/intrinsics in [vmSymbols.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/vmSymbols.hpp#L53) and [vmIntrinsics.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/vmIntrinsics.hpp#L108):

#include "classfile/vmClasses.hpp"
#include "classfile/vmIntrinsics.hpp"
#include "classfile/vmSymbols.hpp" // NEW

Symbol* s = vmSymbols::java_lang_Object();
vmIntrinsics::ID id = vmIntrinsics::_linkToSpecial;
Klass* k = vmClasses::Object_klass(); // NEW

Note: to help the review process and limit the amount of boiler-place changes, within this RFE, we do not change existing calls like `SystemDictionary::Object_klass()` to `vmClasses::Object_klass()`. That will be done in a subtask ([JDK-8260471](https://bugs.openjdk.java.net/browse/JDK-8260471) -- [preliminary webrev](http://cr.openjdk.java.net/~iklam/jdk17/8260471-decouple-vmClasses-and-SystemDictionary/)).

Within this RFE, we temporarily subclass `SystemDictionary` from `vmClasses`, so `SystemDictionary::Object_klass()` will continue to work.

Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero.
The name `vmClasses` is in `lowerCamelCase`, which is a deviation from the rest of the HotSpot code, where most classes have `UpperCamelCase` names. However, `vmClasses` and `vmSymbols` are often used together, like the following, so a consistent look would be nice:

JavaCalls::call_static(&result,
vmClasses::MethodHandleNatives_klass(),
vmSymbols::linkMethodHandleConstant_name(),
vmSymbols::linkMethodHandleConstant_signature(),
&args, CHECK_(empty));

I agree!

dholmes-ora

Hi Ioi,

This first step at refactoring and renaming looks good.

Only one comment below.

Thanks,
David

dholmes-ora · 2021-01-27T06:27:51Z

src/hotspot/share/classfile/vmClasses.cpp

+
+#if INCLUDE_CDS
+
+void vmClasses::resolve_shared_class(InstanceKlass* klass, ClassLoaderData* loader_data, Handle domain, TRAPS) {


It isn't at all obvious to me why this functionality was relocated. I don't see anything vmClasses related in here. ??

vmClasses::resolve_shared_class() is a private method and is called from here:

bool vmClasses::resolve(VMClassID id, TRAPS) { InstanceKlass** klassp = &_klasses[as_int(id)]; ... InstanceKlass* k = *klassp; resolve_shared_class(k, ....); <---- HERE }

Maybe the function name is not clear enough. It's meant to "resolve a vmClass from the CDS archive". It used to be the (even more confusingly named) SystemDictionary::quick_resolve() method.

openjdk · 2021-01-27T06:31:09Z

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

8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp

Reviewed-by: dholmes, coleenp

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.

mlbridge · 2021-01-27T08:42:44Z

Mailing list message from David Holmes on hotspot-dev:

On 27/01/2021 5:52 pm, Ioi Lam wrote:

On Wed, 27 Jan 2021 06:27:51 GMT, David Holmes <dholmes at openjdk.org> wrote:

The "well-known classes" API in [systemDictionary.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/systemDictionary.hpp#L103) is for accessing a fixed set of classes defined by the bootstrap loader. The rest of systemDictionary.hpp is for finding/defining arbitrary classes in arbitrary class loaders. For modularity, we should separate these two sets of APIs, and make them independent of each other.

The proposed API follows the existing pattern of accessing known symbols/intrinsics in [vmSymbols.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/vmSymbols.hpp#L53) and [vmIntrinsics.hpp](https://github.com/openjdk/jdk/blob/19b6f61bd2f49f06ef6b8e9b6ecd2fd910070035/src/hotspot/share/classfile/vmIntrinsics.hpp#L108):

#include "classfile/vmClasses.hpp"
#include "classfile/vmIntrinsics.hpp"
#include "classfile/vmSymbols.hpp" // NEW

Symbol* s = vmSymbols::java_lang_Object();
vmIntrinsics::ID id = vmIntrinsics::_linkToSpecial;
Klass* k = vmClasses::Object_klass(); // NEW

Note: to help the review process and limit the amount of boiler-place changes, within this RFE, we do not change existing calls like `SystemDictionary::Object_klass()` to `vmClasses::Object_klass()`. That will be done in a subtask ([JDK-8260471](https://bugs.openjdk.java.net/browse/JDK-8260471) -- [preliminary webrev](http://cr.openjdk.java.net/~iklam/jdk17/8260471-decouple-vmClasses-and-SystemDictionary/)).

Within this RFE, we temporarily subclass `SystemDictionary` from `vmClasses`, so `SystemDictionary::Object_klass()` will continue to work.

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

src/hotspot/share/classfile/vmClasses.cpp line 219:

217: #if INCLUDE_CDS
218:
219: void vmClasses::resolve_shared_class(InstanceKlass* klass, ClassLoaderData* loader_data, Handle domain, TRAPS) {

It isn't at all obvious to me why this functionality was relocated. I don't see anything vmClasses related in here. ??

`vmClasses::resolve_shared_class()` is a private method and is called from here:

bool vmClasses::resolve(VMClassID id, TRAPS) {
InstanceKlass** klassp = &_klasses[as_int(id)];
...
InstanceKlass* k = *klassp;
resolve_shared_class(k, ....); <---- HERE
}

Maybe the function name is not clear enough. It's meant to "resolve a vmClass from the CDS archive". It used to be the (even more confusingly named) `SystemDictionary::quick_resolve()` method.

Thanks for clarifying - the fact it has a single caller was not clear.

David

coleenp

Why not consolidate vmClassMacros.hpp into vmClassID.hpp ?

iklam · 2021-01-27T18:05:47Z

Why not consolidate vmClassMacros.hpp into vmClassID.hpp ?

ciEnv.hpp uses the VM_CLASSES_DO macro, but does not use anything inside vmClassID.hpp. As a result, vmClassMacros.hpp is included by 468 .o files, but vmClassID.hpp is included only by 318 files.

coleenp

Ok!

…tem-dictionary

iklam · 2021-01-28T04:23:32Z

Thanks @coleenp and @dholmes-ora for the review!
/integrate

openjdk · 2021-01-28T04:24:27Z

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

62ffe75: Merge
e28e111: 8260370: C2: LoopLimit node is not eliminated

Your commit was automatically rebased without conflicts.

Pushed as commit 396a496.

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

iklam added 4 commits January 24, 2021 20:30

Move well_known_classes out of SystemDictionary

42247a5

Renamed VMClasses to vmClasses to be consistent with vmSymbols

8899883

added missing vmClasses.hpp

4278dc8

Merge branch 'master' into 8260467-move-well-known-klasses-out-of-sys…

3d38039

…tem-dictionary

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

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

iklam changed the title ~~8260467: move well known klasses out of system dictionary~~ 8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp Jan 26, 2021

iklam marked this pull request as ready for review January 26, 2021 23:48

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

dholmes-ora approved these changes Jan 27, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 27, 2021

coleenp reviewed Jan 27, 2021

View reviewed changes

coleenp approved these changes Jan 27, 2021

View reviewed changes

Merge branch 'master' into 8260467-move-well-known-klasses-out-of-sys…

e585ddc

…tem-dictionary

openjdk bot closed this Jan 28, 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 Jan 28, 2021

shipilev mentioned this pull request Jan 28, 2021

8260579: PPC64 and S390 builds failures after JDK-8260467 #2282

Closed

5 tasks

iklam mentioned this pull request Jan 28, 2021

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

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp #2246

8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp #2246

iklam commented Jan 26, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 26, 2021

openjdk bot commented Jan 26, 2021

iklam commented Jan 26, 2021

openjdk bot commented Jan 26, 2021

mlbridge bot commented Jan 26, 2021 •

edited

Loading

iklam commented Jan 27, 2021

mlbridge bot commented Jan 27, 2021

dholmes-ora left a comment

dholmes-ora Jan 27, 2021

iklam Jan 27, 2021

openjdk bot commented Jan 27, 2021 •

edited

Loading

mlbridge bot commented Jan 27, 2021

coleenp left a comment

iklam commented Jan 27, 2021

coleenp left a comment

iklam commented Jan 28, 2021

openjdk bot commented Jan 28, 2021


		#if INCLUDE_CDS

		void vmClasses::resolve_shared_class(InstanceKlass* klass, ClassLoaderData* loader_data, Handle domain, TRAPS) {

8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp #2246

8260467: Move well-known classes from systemDictionary.hpp to vmClasses.hpp #2246

Conversation

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

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 26, 2021

openjdk bot commented Jan 26, 2021

iklam commented Jan 26, 2021

openjdk bot commented Jan 26, 2021

mlbridge bot commented Jan 26, 2021 • edited Loading

Webrevs

iklam commented Jan 27, 2021

mlbridge bot commented Jan 27, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jan 27, 2021

Choose a reason for hiding this comment

iklam Jan 27, 2021

Choose a reason for hiding this comment

openjdk bot commented Jan 27, 2021 • edited Loading

mlbridge bot commented Jan 27, 2021

coleenp left a comment

Choose a reason for hiding this comment

iklam commented Jan 27, 2021

coleenp left a comment

Choose a reason for hiding this comment

iklam commented Jan 28, 2021

openjdk bot commented Jan 28, 2021

iklam commented Jan 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 26, 2021 •

edited

Loading

openjdk bot commented Jan 27, 2021 •

edited

Loading