RFR: Tsan oop map table hash fix [v2]

[Jiangli Zhou]

On Mon, 19 Aug 2024 18:38:06 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/gc/shared/weakProcessor.inline.hpp line 100:
> 
>> 98: 
>> 99:   TSAN_RUNTIME_ONLY(
>> 100:     MutexLocker mu(TsanOopMap_lock, Mutex::_no_safepoint_check_flag);
> 
> I think we can completely get rid of this lock during GC, by switching to Atomic on `_nworkers_completed`:
> 
> 
> TSAN_RUNTIME_ONLY(
> uint completed = Atomic::add(&_nworkers_completed, 1);
> assert(_nworkers >= completed, "must be");
> if (_nworkers == completed) {
>   TsanOopMap::notify_tsan_for_freed_and_moved_objects();
> }
> )
> 
> 
> We need to declared `_nworkers_completed` as `volatile uint` for using Atomic.

I also considered with `Atomic::add` when making the change and decided to go with `TsanOopMap_lock`. The performance was not a concern for tsan. Using `Atomic::add` for tracking the completed worker count without a lock seemed also be a little simpler however. Since we already have the `TsanOopMap_lock` and there could be some potential cases that we uncover where we want to put the operations under the lock protection, I think it's better to keep using `TsanOopMap_lock` for now. We could optimize this later after more testing and fully stabilizing the system for tsan/JDK 21.

> src/hotspot/share/tsan/tsanOopMapTable.cpp line 73:
> 
>> 71: 
>> 72: bool TsanOopMapTable::add_entry(TsanOopMapTableKey *entry, size_t size) {
>> 73:   assert(TsanOopMap_lock->is_locked(), "sanity check");
> 
> If we get rid of the locking in `WeakProcessor::Task::work()`, this assert could be `SafepointSynchronize::is_at_safepoint()` instead.

Please see above comment.

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

PR Review Comment: https://git.openjdk.org/tsan/pull/23#discussion_r1722380841
PR Review Comment: https://git.openjdk.org/tsan/pull/23#discussion_r1722381207