RFR: 8292741: Convert JvmtiTagMapTable to ResourceHashtable

[David Holmes]

On Mon, 28 Nov 2022 11:35:48 GMT, Afshin Zafari <duke at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiTagMap.cpp line 254:
>> 
>>> 252:       if (obj_tag != current_tag ) {
>>> 253:         hashmap->remove(o);
>>> 254:         hashmap->add(o, obj_tag);
>> 
>> This change is not atomic - is that a problem? The concurrency aspects of using this map are not clear.
>
> add() is supposed to add a new object with tag. There is an assert() in its body that checks it. By removing the assert, add() can be used for updating as well.

Are you suggesting that `add`  can also act as a `replace` operation? I would think we would want a separate method for that.

>> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 95:
>> 
>>> 93:    //if (obj->fast_no_hash_check()) {
>>> 94:    //  return 0;
>>> 95:    //} else {
>> 
>> What are these comments?
>
> Coleen's suggestion for efficiency reasons.

If the comment is meant to suggest some possible future optimisation it should say that.

>> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 102:
>> 
>>> 100:  }
>>> 101: 
>>> 102: bool JvmtiTagMapTable::add(oop obj, jlong tag) {
>> 
>> I'm not seeing that a return value has any use here when it is always expected to be true.
>
> ResourceHashTable::put() returns true if the Key,Value is added, false if the Value is updated.

But this doesn't do that, so ??

>> src/hotspot/share/prims/jvmtiTagMapTable.cpp line 123:
>> 
>>> 121: 
>>> 122: void JvmtiTagMapTable::remove_dead_entries(GrowableArray<jlong>* objects) {
>>> 123:   struct IsDead{
>> 
>> Nit: space before {
>> 
>> Same query about using a local struct for this.
>
> Alternative for struct?

Not sure ... didn't we start using C++ lambda's for some of these "closure" operations? @coleenp what is the usual pattern we use for this kind of thing?

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

PR: https://git.openjdk.org/jdk/pull/11288