[lworld] RFR: 8355560: SetTag/GetTag should consider equal value objects as the same object

[Serguei Spitsyn]

On Fri, 25 Apr 2025 00:39:58 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> Fixes SetTag/GetTag JVMTI functions so they consider equal value object as a single object.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 142:

> 140: }
> 141: 
> 142: static bool equals_oops(oop obj1, oop obj2) {

Nit: Please, consider to rename functions `equals_value_objects` and `equals_oops` to `equal_value_objects` and `equal_oops`

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 148:

> 146: 
> 147:   if (EnableValhalla) {
> 148:     if (obj1 != nullptr && obj2 != nullptr && obj1->klass() == obj2->klass() && obj1->klass()->is_inline_klass()) {

Nit: I guess, the function `is_inline_type()` can be used instead of `klass()->is_inline_klass()`.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 153:

> 151:     }
> 152:   }
> 153:   return true;

It looks like we have to return `false` here.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 162:

> 160:     return equals_oops(lhs_obj, rhs_obj);
> 161:   }
> 162: 

Nit: The checks at line 159 are not really needed as `equals_oops()` provides the same checks.

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 45:

> 43: //
> 44: // Valhalla: tags for equal ("the same") value object are the same.
> 45: // We have to keep strong reference to each unique value object with non-0 tag.

Nit: A suggestion to slightly change the comment lines above:

  // Valhalla: Keep just one tag for all equal value objects including heap allocated value objects.
  // We have to keep a strong reference to each unique value object with a non-zero tag.

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 48:

> 46: class JvmtiTagMapKey : public CHeapObj<mtServiceability> {
> 47:   // All equal value objects should have the same tag.
> 48:   // So have to keep alive value objects (1 copy for each "value") until their tags are removed.

Nit: A suggestion to slightly modify the comment:

  // Keep value objects alive (1 copy for each "value") until their tags are removed.

test/hotspot/jtreg/serviceability/jvmti/valhalla/SetTag/ValueTagMapTest.java line 46:

> 44: 
> 45:     private static value class ValueClass {
> 46:         public int f1;

Nit: It would be better to have different field name with ValueHolder, e.g. f0. Any other name would also work.

test/hotspot/jtreg/serviceability/jvmti/valhalla/SetTag/libValueTagMapTest.cpp line 40:

> 38: }
> 39: 
> 40: }

Nit: The test library functions `check_jvmti_error()` or `check_jvmti_status()` from `jvmti_common.hpp` can be used instead.
Will need to add this line:

#include "jvmti_common.hpp"

test/hotspot/jtreg/serviceability/jvmti/valhalla/SetTag/libValueTagMapTest.cpp line 57:

> 55:     printf("Could not initialize JVMTI\n");
> 56:     fflush(nullptr);
> 57:     abort();

Nit: The test library macro `LOG()` from `jvmti_common.hpp` can be used. It also calls `fflush()`.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060792568
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060788476
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060785400
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060790750
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060730084
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060737935
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060711636
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060718326
PR Review Comment: https://git.openjdk.org/valhalla/pull/1444#discussion_r2060718953