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

[Man Cao]

On Sat, 27 Jul 2024 00:28:10 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> The new implementation for TsanOopMap support in JDK 21 is partially modeled after [JvmtiTagMap](https://github.com/openjdk/jdk21u-dev/blob/cdbd94f8fa97847c02e1f0f4c6cd75f457a62e25/src/hotspot/share/prims/jvmtiTagMap.hpp#L37) and [JvmtiTagMapTable](https://github.com/openjdk/jdk21u-dev/blob/cdbd94f8fa97847c02e1f0f4c6cd75f457a62e25/src/hotspot/share/prims/jvmtiTagMapTable.hpp#L43):
>> 
>> - Use [WeakHandle](https://github.com/openjdk/jdk21u-dev/blob/cdbd94f8fa97847c02e1f0f4c6cd75f457a62e25/src/hotspot/share/oops/weakHandle.hpp#L42) for handling oops in the table entries;
>> - Use ResizeableResourceHashtable to for the underlying hash table. Table growing/resizing and removal of freed object entries are mostly handled within ResizeableResourceHashtable, except some small parts are done within TsanOopMapTable. That makes the new implementation simpler.
>> 
>> The TsanOopMapTable contains entries consisting of TsanOopMapTableKey:oop_size (key:value) pairs. The TsanOopMapTableKey contains:
>> 
>>  - A Weakhandle that holds a pointer to the oop;
>>  - The original (or updated) address of the object in Java heap;
>> 
>> For TsanOopMapTable support, I added a new weak OopStorage, which is created during tsan_init(). All WeakHandles used for tracking Tsan interested Java objects are created from that OopStorage. GC processes these WeakHandles with `WeakProcessor::Task::work` in concurrent worker threads.
>> 
>> To notify Tsan about object free/move in time, I added a call in WeakProcessor::Task::work to process the TsanOopMap:
>> 
>> - If an object is freed by GC, call __tsan_java_free to notify Tsan. Remove the entry from the table.
>> - If an object is moved by GC, update the raw address in the entry using the new oop address obtained from the WeakHandle and add the “move” to a GrowableArray.
>> - After all entries in TsanOopMap are processed, sort the “moves” in the GrowableArray and notify Tsan by calling __tsan_java_move for each object in the sorted array. This part is the existing implementation from JDK 11 for Tsan support.
>> 
>> Note all above operations occur during GC, before any of the mutators sees a moved or freed Java object.
>> 
>> As an alternative, I initially experimented with doing the above operations concurrently (to mutators) in ServiceThread after GC finished processing the WeakHandles. That could occur after mutators sees moved objects and resulted incorrect data being recorded in Tsan.
>> 
>> Suppressed following Tsan errors in JDK:
>> 1)  In `java.util.concu...
>
> Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision:
> 
>   - Use cast_from_oop<T> in various places when coverting oop to different types.
>   - Change TsanOopMapTableKey::_obj to 'oop'.
>   - Change TsanOopMapTableKey::equals to just do `lhs._obj == rhs._obj`.

Thanks for bearing with me, and addressing issues and testing diligently!

Here are my last set of comments. The one about `_n_downward_moves` is important, and the others are pretty trivial.

src/hotspot/share/tsan/tsanOopMap.cpp line 33:

> 31: #include "memory/resourceArea.hpp"
> 32: #include "oops/oop.inline.hpp"
> 33: #include "runtime/atomic.hpp"

We can probably remove `#include` for `atomic.hpp` and `resizeableResourceHash.hpp`.

src/hotspot/share/tsan/tsanOopMapTable.cpp line 80:

> 78:   } else {
> 79:     size_t* v = _table.put_if_absent(new_entry, size, &added);
> 80:     *v = size;

This can be an assert: `assert(*v == size, "same oop should have same size")`.
This corresponds to `assert(s == bucket->get_oop_size())` in `TsanOopSizeMap::put()`.

src/hotspot/share/tsan/tsanOopMapTable.cpp line 85:

> 83:   if (added) {
> 84:     if (_table.maybe_grow(true /* use_large_table_sizes */)) {
> 85:       log_info(tsan)("TsanOopMapTable resize to %d, %d entries",

Could it be `log_debug(tsan)` as well?

src/hotspot/share/tsan/tsanOopMapTable.cpp line 150:

> 148:         *_dest_high = MAX2(*_dest_high, move.target_end());
> 149:         if (*_dest_low < *_src_low) {
> 150:           ++(*_n_downward_moves);

This seems incorrect. Increment to `_n_downward_moves` should be based on the current move, but `*_dest_low` and `*_src_low` are the minimum of all moves so far. It should be:
`if (move.target_begin() < move.source_begin())`

An inaccurate value of  `_n_downward_moves` only affects the efficiency of `handle_overlapping_moves()`, but not correctness.

src/hotspot/share/tsan/tsanOopMapTable.cpp line 153:

> 151:         }
> 152: 
> 153:         entry.update_obj();

(Just a comment, no action required.)
It is a bit unusual to mutate the key object of a hash table during iteration, as it could mess up key's hash value. Then subsequent `add`, `contains` operations may not find this existing key. Old `TsanOopSizeMap` implementation avoids this problem by creating a new hash table and add all remaining entries there.

However, it is safe here because `TsanOopMapTable` uses `identity_hash()`, so the hash value won't change. This also means enabling TSAN will invoke `identity_hash()` on all Java objects, which may have other side effects, but is probably OK.

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

Changes requested by manc (Reviewer).

PR Review: https://git.openjdk.org/tsan/pull/19#pullrequestreview-2202918789
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1693728785
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1693747331
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1693749070
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1693733272
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1693734105