On Thu, 27 Apr 2023 20:33:38 GMT, Cesar Soares Lucas <[email protected]>
wrote:
>> src/hotspot/share/code/debugInfo.hpp line 199:
>>
>>> 197: // ObjectValue describing an object that was scalar replaced.
>>> 198:
>>> 199: class ObjectMergeValue: public ObjectValue {
>>
>> I find the decision to subclass`ObjectValue` confusing and error prone: now
>> `is_object()` returns true for `ObjectMergeValue`, but you have to apply the
>> selector first to turn it into `ObjectValue`. And now the order of checks
>> matter, so you always have to perform `is_object_merge()` first and then
>> follow it with `is_object()` guard.
>>
>> You have 3 flavors of `ObjectValue` now:
>> * good old `ObjectValue`;
>> * `ObjectMergeValue`
>> * merge candidates (`ObjectMergeCandidateValue`?)
>>
>> Does it make sense to introduce 3 different subclasses under `ObjectValue`
>> to clearly distinguish the scenarios?
>
> Hi @iwanowww . I finished implementing a version of this like the
> illustration below (I didn't add a Candidate class).
>
>
> ScopeValue
> ObjectValue
> ObjectAllocationValue
> AutoBoxObjectValue
> ObjectMergeValue
>
>
> Here are some observations:
>
> - I don't think ObjectMergeValue should be under ObjectValue. The two classes
> only have two fields in common (_id and _visited). I think it should be a
> subclass of ScopeValue.
> - ObjectCandidateValue would need to go under ObjectAllocationValue because
> it essentially _is_ an ObjectAllocationValue in most aspects.
> - I didn't add a ObjectCandidateValue class because that class would need to
> go under ObjectAllocationValue and we would still need to do an
> "is_object_candidate" before all "is_object_allocation" and we would end up
> in much the situation that we want to avoid - needing to do is_object_merge
> before is_object.
> - It seems the best place to flag an object as candidate is really in
> ObjectAllocationValue.
>
> What do you think? As I said, I already have the code, if you want I can push
> it and you take a look.
Can `ObjectCandidateValue` be a wrapper around a `ObjectAllocationValue`?
It does make sense to separate `ObjectMergeValue` and `ObjectValue`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12897#discussion_r1179798496
