Ctor debug #1834
Ctor debug #1834
Conversation
|
👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| } | ||
|
|
||
| ref = commonRef_idToRef(env, id); | ||
| (void)outStream_writeInt(out, objectHashCode(ref)); |
There was a problem hiding this comment.
Interesting that the debug agent already has an objectHashCode() but there were no users of it.
There was a problem hiding this comment.
ObjectReferenceImpl.hashCode() calculates it by itself from objectID (it should call objectHashCode)
| @@ -147,8 +147,16 @@ public boolean vmNotSuspended(VMAction action) { | |||
|
|
|||
| public boolean equals(Object obj) { | |||
| if (obj instanceof ObjectReferenceImpl other) { | |||
| return (ref() == other.ref()) && | |||
| super.equals(obj); | |||
| if (ref() == other.ref() && super.equals(obj)) { | |||
There was a problem hiding this comment.
I'm wondering if the super.equals(obj) call is necessary. The way JDI is implemetned, there is a 1-to-1 relationship between the ObjectID and the ObjectReference. If the ObjectIDs match, then the ObjectRefernces instances should be one in the same. So either one of these checks should be sufficient. I don't think both are needed.
There was a problem hiding this comment.
I agree that super.equals call is not necessary, just kept it as it was
| try { | ||
| return JDWP.ObjectReference.IsSameObject.process(vm, this, other).isSameObject; | ||
| } catch (JDWPException exc) { | ||
| throw exc.toJDIException(); | ||
| } |
There was a problem hiding this comment.
So the downside here is that when we don't have equality w.r.t. ObjectIDs, we do the heavyweight call over JDWP. A while back I did a scan of users of equals() in the JDI implementation and jdb, and it seemed it was always done on known subtypes (ClassLoaderReference, ThreadReference, ClassReference, etc). So I doubt in practice you would ever see two value types being compared, unless the debugger was doing it for some reason. So if you did limit this JDWP call to value types, probably it would be rare to non-existent that it would ever be called.
There was a problem hiding this comment.
agreed. Do I understand correctly that I cannot modify existing command (jdwp protocol shoul be backward compatible?) to get classes and need a new one to check if the class is value class?
There was a problem hiding this comment.
I'm not sure what your backward compatible question is. I think we need an API to determine if a ClassType is a value type or not. This could be expensive to maintain. Probably we need to only look it up when needed and can keep it cached with the ClassType. Another option is to only try to avoid the IsSameObject callback when we are not dealing with an ObjectReferenceImpl subclass. We can make a check something like obj.getClass() == ObjectReferenceImpl.class. When they are not equal, then we are dealing with a subclass that we know can't be a value type.
| {referringObjects, "ReferringObjects"}, | ||
| {isSameObjectImpl, "IsSameObject"}, | ||
| {objectHashCodeImpl, "ObjectHashCode"} | ||
| }; |
There was a problem hiding this comment.
I'd suggest to keep current naming convention (just first letter is changed to capital):
referenceType <=> ReferenceType
disableCollection <=> DisableCollection
referringObjects <=> ReferringObjects
isSameObjectIs <=> SameObject
objectHashCode <=> ObjectHashCode
Then the existing objectHashCode needs to be renamed to something else.
There was a problem hiding this comment.
We have isSameObject too (and it's used in many places)
3 participants
Progress
Error
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1834/head:pull/1834$ git checkout pull/1834Update a local copy of the PR:
$ git checkout pull/1834$ git pull https://git.openjdk.org/valhalla.git pull/1834/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1834View PR using the GUI difftool:
$ git pr show -t 1834Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1834.diff