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

[Man Cao]

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

>> 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...
>
> 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.

We looked into this offline. Yes, the failure is indeed due to the missing call to `notify_tsan_for_freed_and_moved_objects()` in `G1FullGCAdjustTask` and `PSAdjustTask`. In particular, in G1 and Parallel GC's full collector, we have to process TSAN oop map at least twice:
1. In Phase 1 after marking. This iteration gets rid of dead objects from TSAN oop map, but it does not process to-be-moved objects, because the destination addresses (forwarding pointers) are not yet available.
2. In Phase 3 after or during adjusting pointers. This iteration processes to-be-moved objects in TSAN oop map, as forwarding pointers are available during this phase.

In JDK 21, we cannot find a shared function in weak processor that is executed by VM thread from G1 and Parallel collector's adjusting pointers phase. We have two options:

a. Keep Jiangli's current implementation, that the TSAN oop map is iterated by multiple GC worker threads. The worker threads need to hold `TsanOopMap_lock`. Also note that `notify_tsan_for_freed_and_moved_objects()` needs to run after the existing code in `WeakProcessor::Task::work` iterates through weak OopStorage in `OopStorageSet` by all worker threads. It appears difficult to correctly make `notify_tsan_for_freed_and_moved_objects()` called by only one GC worker thread.

b. Insert `notify_tsan_for_freed_and_moved_objects()` to G1 and Parallel GC's full collector (`G1FullCollector::phase3_adjust_pointers()` and `PSParallelCompact::adjust_roots()`), in addition to the two versions of `WeakProcessor::weak_oops_do()`. The downside is that we will be modifying code specific to a garbage collector.

It is not obvious which approach will be easier to maintain in the long term. I slightly prefer `b.`, but `a.` is also OK. The performance downside of `a.` is not a big concern as TSAN is not optimized for performance.

Also note that TSAN currently is only tested under G1, and it is unknown if it works correctly under ParallelGC. I'll update TSAN's wiki page to document this. With this change, for approach `a.` TSAN won't work under serial GC; for `b.` TSAN probably does not work for Shenandoah. Neither approach is likely to work under ZGC or XGC.

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

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