8285364: Remove REF_ enum for java.lang.ref.Reference #8332

albertnetymk · 2022-04-21T10:48:06Z

Simple rename and some comments update.

Test: build

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8285364: Remove REF_ enum for java.lang.ref.Reference

Reviewers

Kim Barrett (@kimbarrett - Reviewer)
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 73f80ea4
Stefan Karlsson (@stefank - Reviewer)

Contributors

Stefan Karlsson <stefank@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8332/head:pull/8332
$ git checkout pull/8332

Update a local copy of the PR:
$ git checkout pull/8332
$ git pull https://git.openjdk.org/jdk pull/8332/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8332

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8332.diff

bridgekeeper · 2022-04-21T10:48:28Z

👋 Welcome back ayang! 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 · 2022-04-21T10:51:31Z

@albertnetymk 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.

mlbridge · 2022-04-21T10:56:07Z

Webrevs

kimbarrett · 2022-04-21T11:11:44Z

src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp

@@ -214,7 +214,7 @@ void MetaspaceObjectTypeConstant::serialize(JfrCheckpointWriter& writer) {
 static const char* reference_type_to_string(ReferenceType rt) {
  switch (rt) {
    case REF_NONE: return "None reference";
-    case REF_OTHER: return "Other reference";
+    case REF_REFERENCE: return "j.l.r.Reference";


I think either "Reference" or "java.lang.ref.Reference" would be better than the abbreviation.

kimbarrett · 2022-04-21T11:13:39Z

src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp

@@ -214,7 +214,7 @@ void MetaspaceObjectTypeConstant::serialize(JfrCheckpointWriter& writer) {
 static const char* reference_type_to_string(ReferenceType rt) {


This function seems misplaced here. Seems like it belongs with the ReferenceType type. (This could be a followup RFE.)

kimbarrett · 2022-04-21T11:15:41Z

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/ReferenceType.java

@@ -28,7 +28,7 @@

 public enum ReferenceType {
  REF_NONE ("None reference"),       // Regular class
-  REF_OTHER ("Other reference"),     // Subclass of java/lang/ref/Reference, but not subclass of one of the classes below
+  REF_REFERENCE ("j.l.r.Reference"), // java/lang/ref/Reference, super class of the following


Again here, don't abbreviate the package path.

stefank · 2022-04-21T11:36:35Z

Do we even need REF_OTHER / REF_REFERENCE? I see a bunch of ShouldNotReachHeres and then set_reference_type(REF_OTHER). Maybe the latter could be changed to REF_NONE?

albertnetymk · 2022-04-21T11:44:19Z

Maybe the latter could be changed to REF_NONE?

vmClasses::Reference_klass()->set_reference_type(REF_NONE); fails in make images.

The relevant code is:

ClassFileParser::post_process_parsed_stream(...) {
  ...
  _rt = (NULL ==_super_klass) ? REF_NONE : _super_klass->reference_type();
}

InstanceKlass::allocate_instance_klass(...) {
  ...
  if (REF_NONE == parser.reference_type()) {
    ...
  } else {
    // reference
    ik = new (loader_data, size, THREAD) InstanceRefKlass(parser);
  }
}

The else branch is for non-strong refs and relies on the ref-type of their super klass not being REF_NONE.

stefank · 2022-04-21T12:19:20Z

That's unfortunate. The allocate_instance_klass function can easily be changed to also check if class_name == vmSymbols::java_lang_ref_Reference(). But even with that, make images fails.

I wasn't a fan of the REF_REFERENCE name, since it isn't really that descriptive to me, and had hoped to fine a way to just get rid of it instead of finding a better name.

albertnetymk · 2022-04-21T15:08:45Z

had hoped to fine a way to just get rid of it instead of finding a better name.

vmClasses::Reference_klass()->set_reference_type(REF_SOFT); works. In fact, anything other than REF_NONE works. The correct ref-type for each subclass of Reference will be set later on in vmClasses::resolve_all. Do you think that is better?

stefank · 2022-04-22T08:03:42Z

Using REF_SOFT seems too hacky. If we rewrite the initialization a bit we can make this work. See suggested change in:
https://github.com/stefank/jdk/tree/ref-rename
stefank@68161ce

albertnetymk · 2022-04-22T08:22:55Z

Using REF_SOFT seems too hacky.

Just to put all alternatives on the table. The use of REF_SOFT is ephemeral.

  // in vmClasses::resolve_all
  vmClasses::Reference_klass()->set_reference_type(REF_SOFT);
  
  // setting ref-type of Soft/Weak/Final/Phantom
  vmClasses::SoftReference_klass()->set_reference_type(REF_SOFT);
  ...
  
  vmClasses::Reference_klass()->set_reference_type(REF_NONE);

I have no particular preference. What does everyone think?

kimbarrett · 2022-04-22T09:04:37Z

Using REF_SOFT seems too hacky.

Just to put all alternatives on the table. The use of REF_SOFT is ephemeral.

[...]
I have no particular preference. What does everyone think?

I also think using REF_SOFT like that is overly hacky.

I think there is still a bootstrapping weirdness, whether using REF_REFERENCE
or using Stefan's approach. I think Soft/Weak/Final/PhantomReference_klass get
constructed with the reftype of Reference_klass (whatever that is), and then
have it later updated to the correct value. In that respect REF_OTHER seems
better than REF_REFERENCE. So I no longer like the renaming. Getting rid of it
entirely with Stefan's approach, they get REF_NONE initially and then set to
the proper value; that's far from the worst bootstrapping kludge I've ever seen.

Signed-off-by: Albert Yang <albert.m.yang@oracle.com>

albertnetymk · 2022-04-22T09:42:32Z

I have taken the commit from Stefan and updated the title.

albertnetymk · 2022-04-22T09:45:07Z

/contributor add stefank

openjdk · 2022-04-22T09:45:59Z

@albertnetymk
Contributor Stefan Karlsson <stefank@openjdk.org> successfully added.

stefank · 2022-04-22T16:01:26Z

Using REF_SOFT seems too hacky.

Just to put all alternatives on the table. The use of REF_SOFT is ephemeral.
[...]
I have no particular preference. What does everyone think?

I also think using REF_SOFT like that is overly hacky.

I think there is still a bootstrapping weirdness, whether using REF_REFERENCE or using Stefan's approach. I think Soft/Weak/Final/PhantomReference_klass get constructed with the reftype of Reference_klass (whatever that is), and then have it later updated to the correct value. In that respect REF_OTHER seems better than REF_REFERENCE. So I no longer like the renaming. Getting rid of it entirely with Stefan's approach, they get REF_NONE initially and then set to the proper value; that's far from the worst bootstrapping kludge I've ever seen.

I think we can fix the bootstrapping kludge with something like this:
stefank@e62ddb7

With this patch, we'll always have the correct _reference_type after the InstanceRefKlass constructor has run.

(Patch has not gone through full testing yet)

coleenp · 2022-04-25T15:51:35Z

src/hotspot/share/oops/instanceKlass.cpp

-      ik = new (loader_data, size, THREAD) InstanceKlass(parser);
-    }
-  } else {
+  if (parser.is_java_lang_ref_Reference_subclass()) {


I'm having a really hard time understanding this. java.lang.Reference now doesn't have any REF_OTHER type set. I didn't realize that a java.lang.Reference instance is a plain InstanceKlass, and not an InstanceRefKlass. Is this right?

Yes, I was also surprised by this. The java.lang.ref.Reference Klass is only an InstanceKlass, not an InstanceRefKlass.

Yeah, please add a short comment in the is_java_lang_ref_Reference_subclass function that this is the case, now that we've learned this.

Instead of documenting this surprise, how about removing it by extending this method to cover all subtypes of j.l.r.Reference?

bool ClassFileParser::is_java_lang_ref_Reference_or_subclass() const { if (_super_klass == NULL) { return false; } if (_class_name == vmSymbols::java_lang_ref_Reference()) { return true; } if (_super_klass->name() == vmSymbols::java_lang_ref_Reference()) { return true; } return _super_klass->reference_type() != REF_NONE; }

coleenp

I think this is better.

openjdk · 2022-04-27T12:37:41Z

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

8285364: Remove REF_ enum for java.lang.ref.Reference

Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Reviewed-by: kbarrett, coleenp, stefank

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 111 new commits pushed to the master branch:

167ce4d: 8289421: No-PCH build for Minimal VM was broken by JDK-8287001
108cd69: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long
b96ba19: 8289182: NMT: MemTracker::baseline should return void
779b4e1: 8287001: Add warning message when fail to load hsdis libraries
910053b: 8280235: Deprecated flag FlightRecorder missing from VMDeprecatedOptions test
7b3bf97: 8289401: Add dump output to TestRawRSACipher.java
86dc760: Merge
1504804: 8289398: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 again
9b7805e: 8289069: Very slow C1 arraycopy jcstress tests after JDK-8279886
c42b796: 8288058: Broken links on constant-values page
... and 101 more: https://git.openjdk.org/jdk/compare/f0804304400f380661a8a06e4c1dad97068fae60...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

Signed-off-by: Albert Yang <albert.m.yang@oracle.com>

albertnetymk · 2022-04-28T09:11:13Z

I have taken "Rework reference type initialization" from Stefan and added comments on a potential surprise that j.l.r.Reference is instanceKlass not instanceRefKlass.

kimbarrett

There are further cleanups possible if we didn't have REF_NONE and InstanceKlass didn't have a reference type. But there are a number of uses of InstanceKlass::reference_type. Doing anything along those lines (assuming it's deemed worthwhile to do so) can be done in a followup RFE.

kimbarrett · 2022-04-28T10:49:47Z

src/hotspot/share/oops/instanceRefKlass.cpp

+    return rt;
+  }
+
+  // Bootstrapping: this is either of the four direct subclasses of java.lang.ref.Reference


s/either of the four/one of the/. In particular remove "four" else, someday, finalization will finally be GC'd and this comment will need to be noticed and updated.

kimbarrett · 2022-04-28T11:00:11Z

src/hotspot/share/oops/instanceKlass.cpp

@@ -496,7 +494,7 @@ InstanceKlass::InstanceKlass(const ClassFileParser& parser, KlassKind kind) :
  _itable_len(parser.itable_size()),
  _nest_host_index(0),
  _init_state(allocated),
-  _reference_type(parser.reference_type()),
+  _reference_type(REF_NONE),


This is initializing _reference_type to the wrong value for a InstanceRefKlass object, which then needs to reset it in the derived constructor. Why not get the reference type from the parser? The (currently file-scoped static) determine_reference_type function in instanceRefKlass.cpp doesn't have any dependency on the klass object being constructed, just the parser.

The current approach limits the knowledge of non-strong ref types to instanceRefKlass file.

The _reference_type used to be initialized correctly in most cases, but
needed fixing up for a few cases during bootstrapping. With this change it is
never initialized correctly for Reference subtypes and always needs an
initialization kludge for them. That's not an improvement.

_reference_type always gets the correct value after the constructor is run. The member initializer list just follows the convention of having all fields set. One could move this field inside the constructor body to ensure this field is set only once.

The current approach limits the knowledge of non-strong ref types to instanceRefKlass file.

The mechanism used to compute the correct reftype is purely derived from the
parser, and to me looks like "parsing". So I think it would be better placed
with the parser. And that would also remove the need for the two-step
initialization of the InstanceKlass member.

With that in mind, I think the file scoped function determine_reference_type
in instanceRefKlass.cpp should instead be an ordinary public member function
ClassFileParser::reference_type() const (with appropriate adjustment of it's
implementation), called by the InstanceKlass constructor to initialize
_reference_type.

Associated with that are some other changes:

The new InstanceRefKlass::set_reference_type is no longer needed.

The new ClassFileParser::super_reference_type and
ClassFileParser::is_java_lang_ref_Reference_subclass don't need to be public.
(Assuming they exist at all after adjustment of ClassFileParser::reference_type.)

The _reference_type used to be initialized correctly in most cases, but
needed fixing up for a few cases during bootstrapping. With this change it is
never initialized correctly for Reference subtypes and always needs an
initialization kludge for them. That's not an improvement.

I don't agree that this is a kludge. This is quite common in HotSpot, and the code is quite understandable. It's far better than the initialization kludge the code had before this patch.

I'd like to suggest that we go with the proposed patch, and consider the Kim's latest proposal as a follow-up RFE. That way we can make progress on this RFE, and then weigh the pros and cons of this suggestion separately.

Talked to Albert. He's already created a patch for this, so I retract my objection to this blocking of the PR.

Revised as Kim suggested.

kimbarrett · 2022-04-28T11:01:04Z

src/hotspot/share/oops/instanceKlass.hpp

@@ -574,11 +574,16 @@ class InstanceKlass: public Klass {

  // reference type
  ReferenceType reference_type() const     { return (ReferenceType)_reference_type; }
+
+protected:
+  // Only used by the InstanceRefKlass constructor
  void set_reference_type(ReferenceType t) {


This function wouldn't be needed at all if the reference type was properly initialized.

kimbarrett · 2022-04-28T11:05:45Z

src/hotspot/share/classfile/classFileParser.cpp

+  }
+
+  return _super_klass->reference_type() != REF_NONE;
+}


Stylistically, I'd prefer an if-ladder here. I might also swap the reference-type check and the name check, so the for-bootstrapping name check case being last (with a comment to that effect).

These if-checks more or less mirror the type hierarchy, Object -> Reference -> Soft|Weak... -> .... Then, walking down the type hierarchy using the early-return style here makes more sense to me.

bridgekeeper · 2022-06-06T09:26:15Z

@albertnetymk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

kimbarrett

Looks good.

Just a couple of minor "naming" comments, that you can address or not.

kimbarrett · 2022-06-28T00:47:07Z

src/hotspot/share/classfile/classFileParser.cpp

+  // klass requiring special treatment in ref-processing. The abstract
+  // j.l.r.Reference cannot be instantiated so doesn't partake in
+  // ref-processing.
+  return is_java_lang_ref_Reference_subclass();


This seems to be the only call to this function, so the definition could just be inlined here and drop the separate function.

kimbarrett · 2022-06-28T00:49:59Z

src/hotspot/share/classfile/classFileParser.hpp

@@ -561,7 +562,9 @@ class ClassFileParser {
  const Symbol* class_name() const { return _class_name; }
  const InstanceKlass* super_klass() const { return _super_klass; }

-  ReferenceType reference_type() const { return _rt; }
+  bool is_instance_ref_klass() const;
+  ReferenceType determine_reference_type() const;


I'd prefer this was just called reference_type. Like much of the API here, this is about a property of the designated klass. That it's no longer just a data member accessor is of no particular importance.

stefank

Looks good. +1 on Kim's latest comments.

stefank

Scratch my last comment. This crashes in GHA, because determine_reference_type() now gets called from non-InstanceRefKlass InstanceKlasses, which is wasn't written to support.

kimbarrett

Looks good.

albertnetymk · 2022-06-29T13:13:50Z

Thank you for the help and review.

/integrate

openjdk · 2022-06-29T13:15:31Z

Going to push as commit 2961b7e.
Since your change was applied there have been 111 commits pushed to the master branch:

167ce4d: 8289421: No-PCH build for Minimal VM was broken by JDK-8287001
108cd69: 8283726: x86_64 intrinsics for compareUnsigned method in Integer and Long
b96ba19: 8289182: NMT: MemTracker::baseline should return void
779b4e1: 8287001: Add warning message when fail to load hsdis libraries
910053b: 8280235: Deprecated flag FlightRecorder missing from VMDeprecatedOptions test
7b3bf97: 8289401: Add dump output to TestRawRSACipher.java
86dc760: Merge
1504804: 8289398: ProblemList jdk/jfr/api/consumer/recordingstream/TestOnEvent.java on linux-x64 again
9b7805e: 8289069: Very slow C1 arraycopy jcstress tests after JDK-8279886
c42b796: 8288058: Broken links on constant-values page
... and 101 more: https://git.openjdk.org/jdk/compare/f0804304400f380661a8a06e4c1dad97068fae60...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-29T13:16:00Z

@albertnetymk Pushed as commit 2961b7e.

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

ref-rename

6423f0a

openjdk bot changed the title ~~8285364~~ 8285364: Use more precise name for ReferenceType::REF_OTHER Apr 21, 2022

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

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

kimbarrett suggested changes Apr 21, 2022

View reviewed changes

review

746ff8e

Remove REF_ enum for java.lang.ref.Reference

73f80ea

Signed-off-by: Albert Yang <albert.m.yang@oracle.com>

albertnetymk changed the title ~~8285364: Use more precise name for ReferenceType::REF_OTHER~~ 8285364: Remove REF_ enum for java.lang.ref.Reference Apr 22, 2022

coleenp reviewed Apr 25, 2022

View reviewed changes

coleenp approved these changes Apr 27, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 27, 2022

stefank and others added 2 commits April 28, 2022 10:45

Rework reference type initialization

0d70b49

Signed-off-by: Albert Yang <albert.m.yang@oracle.com>

comment

e4f6a66

kimbarrett suggested changes Apr 28, 2022

View reviewed changes

albertnetymk added 2 commits April 28, 2022 22:06

review

4b9081b

merge

196a260

albertnetymk added 3 commits June 21, 2022 20:45

merge

3fa67ab

fix build

b1c0369

review

f7a5f1e

kimbarrett approved these changes Jun 28, 2022

View reviewed changes

stefank approved these changes Jun 28, 2022

View reviewed changes

stefank suggested changes Jun 28, 2022

View reviewed changes

albertnetymk added 4 commits June 28, 2022 13:20

fix-build

3a3e4d0

revert

cd624be

default arg

e0d1ef8

arg rename

1e4c71a

kimbarrett approved these changes Jun 28, 2022

View reviewed changes

stefank approved these changes Jun 29, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 29, 2022

openjdk bot closed this Jun 29, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 29, 2022

albertnetymk deleted the ref-rename branch June 29, 2022 13:26

		@@ -214,7 +214,7 @@ void MetaspaceObjectTypeConstant::serialize(JfrCheckpointWriter& writer) {
		static const char* reference_type_to_string(ReferenceType rt) {

8285364: Remove REF_ enum for java.lang.ref.Reference #8332

8285364: Remove REF_ enum for java.lang.ref.Reference #8332

Conversation

albertnetymk commented Apr 21, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 21, 2022

openjdk bot commented Apr 21, 2022

mlbridge bot commented Apr 21, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank commented Apr 21, 2022

albertnetymk commented Apr 21, 2022

stefank commented Apr 21, 2022

albertnetymk commented Apr 21, 2022

stefank commented Apr 22, 2022

albertnetymk commented Apr 22, 2022

kimbarrett commented Apr 22, 2022

albertnetymk commented Apr 22, 2022

albertnetymk commented Apr 22, 2022

openjdk bot commented Apr 22, 2022

stefank commented Apr 22, 2022

Choose a reason for hiding this comment

stefank Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 27, 2022 • edited Loading

albertnetymk commented Apr 28, 2022

kimbarrett 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

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

bridgekeeper bot commented Jun 6, 2022

kimbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

albertnetymk commented Jun 29, 2022

openjdk bot commented Jun 29, 2022

openjdk bot commented Jun 29, 2022

albertnetymk commented Apr 21, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 21, 2022 •

edited

Loading

stefank Apr 25, 2022 •

edited

Loading

openjdk bot commented Apr 27, 2022 •

edited

Loading