RFR: Reimplement TsanOopMap support using WeakHandle and ResizeableResourceHashtable [v2]

[Jiangli Zhou]

On Tue, 23 Jul 2024 20:38:20 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>>> The WeakProcessor::Task::work() function is called by a subset of GC worker thread. This function will be called multiple times in parallel, but each thread runs the same code and processes the same data, so it is redundant work. It should probably be called only once, by the VM thread.
>> 
>> Yes, that's correct. I've noticed it have been thinking updating this part already.
>> 
>>> We can invoke notify_tsan_for_freed_and_moved_objects() from the > VM thread by adding it to the two versions of  WeakProcessor::weak_oops_do below:
>>>
>>> After line 142 task.report_num_dead(), in this file (weakProcessor.inline.hpp).
>>> In weakProcessor.cpp, between inside  WeakProcessor::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive).
>> 
>> I think we only need to add `notify_tsan_for_freed_and_moved_objects()` call in `void WeakProcessor::weak_oops_do(WorkerThreads* workers, IsAlive* is_alive, KeepAlive* keep_alive,WeakProcessorTimes* times)` in weakProcessor.inline.hpp. I think that should be able to cover all cases.
>> 
>> `WeakProcessor::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive)` in weakProcessor.cpp is only used by serial GC. We can add the call in there, but it would not be fully tested.
>
> My further testing indicates moving `notify_tsan_for_freed_and_moved_objects()` from `WeakProcessor::Task::work()` to `WeakProcessor::weak_oops_do` calls cannot cover all cases.
> 
> 
>  stdout: [];
>  stderr: [ThreadSanitizer: CHECK failed: tsan_sync.cpp:261 "((*dst_meta)) == ((0))" (0x80000238, 0x0) (tid=709887)
>     #0 __tsan::CheckUnwind() <null> (java+0xbc4ff) (BuildId: 64a25ac2795c9dd62d0a35fdf577c14b358e86be)
>     #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) <null> (java+0x34b26) (BuildId: 64a25ac2795c9dd62d0a35fdf577c14b358e86be)
>     #2 __tsan::MetaMap::MoveMemory(unsigned long, unsigned long, unsigned long) <null> (java+0xd40c7) (BuildId: 64a25ac2795c9dd62d0a35fdf577c14b358e86be)
>     #3 __tsan_java_move <null> (java+0xb2919) (BuildId: 64a25ac2795c9dd62d0a35fdf577c14b358e86be)
>     #4 TsanOopMap::notify_tsan_for_freed_and_moved_objects() make/hotspot/src/hotspot/share/tsan/tsanOopMap.cpp:326:9 (libjvm.so+0xe1e5a2) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #5 void WeakProcessor::weak_oops_do<G1IsAliveClosure, DoNothingClosure>(WorkerThreads*, G1IsAliveClosure*, DoNothingClosure*, WeakProcessorTimes*) make/hotspot/src/hotspot/share/gc/shared/weakProcessor.inline.hpp:145:3 (libjvm.so+0x73d991) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #6 void WeakProcessor::weak_oops_do<G1IsAliveClosure, DoNothingClosure>(WorkerThreads*, G1IsAliveClosure*, DoNothingClosure*, unsigned int) make/hotspot/src/hotspot/share/gc/shared/weakProcessor.inline.hpp:159:3 (libjvm.so+0x73645c) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #7 G1FullCollector::phase1_mark_live_objects() make/hotspot/src/hotspot/share/gc/g1/g1FullCollector.cpp:316:5 (libjvm.so+0x73645c)
>     #8 G1FullCollector::collect() make/hotspot/src/hotspot/share/gc/g1/g1FullCollector.cpp:207:3 (libjvm.so+0x735e07) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #9 G1CollectedHeap::do_full_collection(bool, bool) make/hotspot/src/hotspot/share/gc/g1/g1CollectedHeap.cpp:932:13 (libjvm.so+0x7146d5) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #10 VM_G1CollectFull::doit() make/hotspot/src/hotspot/share/gc/g1/g1VMOperations.cpp:54:24 (libjvm.so+0x7816e1) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #11 VM_Operation::evaluate() make/hotspot/src/hotspot/share/runtime/vmOperations.cpp:71:3 (libjvm.so+0xe8bd32) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
>     #12 VMThread::evaluate_operation(VM_Operation*) ...

Thinking more of  `TsanOopMap::notify_tsan_for_freed_and_moved_objects()` and `TsanOopMapTable::collect_moved_objects_and_notify_freed()`, I think the overhead is minimum.

Each concurrent worker thread iterates the TsanOopMapTable. However, the table entries are immediately updated by the worker thread:

- entry is removed when a freed object is encountered
- entry is updated to point to the new address when a moved object is encountered

So each entry in the table is actually processed only once. Tsan is also only notified once for any freed or moved object.

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

PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1688700198