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

Jiangli Zhou jiangli at openjdk.org
Tue Jul 23 20:40:56 UTC 2024 

On Tue, 23 Jul 2024 18:55:32 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> src/hotspot/share/gc/shared/weakProcessor.inline.hpp line 100:
>> 
>>> 98: 
>>> 99:   TSAN_RUNTIME_ONLY(
>>> 100:     TsanOopMap::notify_tsan_for_freed_and_moved_objects();
>> 
>> 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.
>> 
>> 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)`.
>> 
>> We have another internal patch that adds a callback during a GC pause, at those two exact places. It is very similar to what TSAN needs to do.
>
>> 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*) make/hotspot/src/hotspot/share/runtime/vmThread.cpp:281:9 (libjvm.so+0xe8db1e) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
    #13 VMThread::inner_execute(VM_Operation*) make/hotspot/src/hotspot/share/runtime/vmThread.cpp:435:3 (libjvm.so+0xe8e13e) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
    #14 VMThread::loop() make/hotspot/src/hotspot/share/runtime/vmThread.cpp:502:5 (libjvm.so+0xe8d829) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
    #15 VMThread::run() make/hotspot/src/hotspot/share/runtime/vmThread.cpp:175:9 (libjvm.so+0xe8d829)
    #16 Thread::call_run() make/hotspot/src/hotspot/share/runtime/thread.cpp:217:9 (libjvm.so+0xe0efe4) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
    #17 thread_native_entry(Thread*) make/hotspot/src/hotspot/os/linux/os_linux.cpp:778:11 (libjvm.so+0xbe9da8) (BuildId: 8cda182fb0a5fe22eb8242eab7d73b7123b7659b)
    #18 __tsan_thread_start_func <null> (java+0x4f2d2) (BuildId: 64a25ac2795c9dd62d0a35fdf577c14b358e86be)
    #19 start_thread nptl/pthread_create.c:444:8 (libc.so.6+0x8845b) (BuildId: b830dcb9c6f0ee1f8679364e0f99ce35db953847)
    #20 clone3 misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 (libc.so.6+0x108bbb) (BuildId: b830dcb9c6f0ee1f8679364e0f99ce35db953847)


Looking more closely now, I see `WeakProcessor::Task::work()` is also used in `G1FullGCAdjustTask::work` and `PSAdjustTask`.  I'm reluctant to add the calls in various different places  in GC code, as that's less maintainable. 

I'm looking into a bit more to see to reduce the overhead by doing it in the concurrent worker thread.

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

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

More information about the tsan-dev mailing list