On Mon, 28 Nov 2022 11:35:48 GMT, Afshin Zafari <[email protected]> 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
