RFR: Tsan oop map table hash fix [v2]

[Jiangli Zhou]

On Thu, 15 Aug 2024 00:33:31 GMT, Man Cao <manc at openjdk.org> wrote:

>> Jiangli Zhou has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - - Release memory for the malloced new TsanOopMapTableKey after adding back to the TsanOopMap. Thanks manc@ for
>>      noticing the memory leak.
>>    - Add TsanOopMapTableKey::TsanOopMapTableKey(const TsanOopMapTableKey& src, oop obj).
>>  - Change to call TsanOopMap::notify_tsan_for_freed_and_moved_objects from the last worker thread that completes
>>    the parallel WeakProcessor::Task work:
>>    - Add WeakProcessor::Task::_nworkers_completed to track the completed workers. This is only used if
>>      INCLUDE_TSAN is true.
>>    - Increment WeakProcessor::Task::_nworkers_completed in WeakProcessor::Task::work when each parallel worker
>>      finishes work.
>>    
>>    With the change, TsanOopMap::notify_tsan_for_freed_and_moved_objects is only called from one thread. This is
>>    to address the issue described in https://github.com/openjdk/tsan/pull/23#discussion_r1717793720.
>
> src/hotspot/share/tsan/tsanOopMap.cpp line 235:
> 
>> 233:       const TsanOopMapImpl::MovedEntry &e = moved_entries.at(i);
>> 234:       _oop_map->add_entry(e.key(), e.value());
>> 235:     }
> 
> Is there a memory leak on the `MovedEntry::TsanOopMapTableKey*` after here?
> `TsanOopMapTable::add_entry()` makes a copy `*(e.key())`, but `e.key()` appears never freed.

Fixed. Thanks for noticing it.

> src/hotspot/share/tsan/tsanOopMapTable.cpp line 161:
> 
>> 159:         }
>> 160: 
>> 161:         entry.update_obj();
> 
> It is probably better to remove the `TsanOopMapTableKey::update_obj()` function altogether, so `TsanOopMapTableKey` becomes an immutable object. There is no point to update the old object when it is going to be removed immediately afterwards.
> 
> The code can use `wh_obj` to create a new `TsanOopMapTableKey`. `TsanOopMapTableKey` may need a new constructor though:
> 
> TsanOopMapTableKey::TsanOopMapTableKey(const TsanOopMapTableKey& src, oop obj) {
>   _wh = src._wh;
>   _obj = obj;
> }

Adding TsanOopMapTableKey::TsanOopMapTableKey(const TsanOopMapTableKey& src, oop obj) and removing TsanOopMapTableKey::update_obj() sounds ok to me. Done.

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

PR Review Comment: https://git.openjdk.org/tsan/pull/23#discussion_r1720531124
PR Review Comment: https://git.openjdk.org/tsan/pull/23#discussion_r1720532042