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

[Man Cao]

On Wed, 24 Jul 2024 18:50:34 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> src/hotspot/share/tsan/tsanOopMapTable.cpp line 49:
>> 
>>> 47: 
>>> 48: void TsanOopMapTableKey::update_obj() {
>>> 49:   oop obj = _wh.peek();
>> 
>> Since `_obj` is `oopDesc*` and `oop` could be an object, is it better to do `oopDesc* obj = _wh.peek()`?
>
> `oopDesc*` is `oop`.

`oopDesc*` is different from `oop` in a fastdebug build, when `CHECK_UNHANDLED_OOPS` is defined.

>> src/hotspot/share/tsan/tsanOopMapTable.hpp line 78:
>> 
>>> 76: 
>>> 77:   static bool equals(const TsanOopMapTableKey& lhs, const TsanOopMapTableKey& rhs) {
>>> 78:     oop lhs_obj = lhs._obj != nullptr ? (oop)lhs._obj : lhs.object_no_keepalive();
>> 
>> I think `lhs._obj` and `rhs._obj` will never be `nullptr` in practice. This is because:
>> - We never add a null oop to the table.
>> - Once `_wh.peek()` returns null, the associated entry is immediately removed. We never update `_obj` to null.
>> 
>> Would it better to add an assert that `lhs._obj` and `rhs._obj` are both non-null and remove the call to `object_no_keepalive()`?
>> 
>> (I find it a bit hard to reason why we would resort to `_wh` for equality. I know `JvmtiTagMapKey::_obj` would be null most of the time in contrast.)
>
> I had also considered to not check if `_obj` is nullptr and decided to keep the check. The main concern is that I don't know if there could be a case where it wants to compare an entry with freed object. I feel it's safer to keep the check.

Could we try adding an assertion? `assert(lhs._obj != nullptr && rhs._obj != nullptr)`.
Even if we run into an entry whose `_obj` is freed, `_wh.peek()` should also return null in that case. So there's no need to look at `_wh`.

>> src/hotspot/share/tsan/tsanOopMapTable.hpp line 104:
>> 
>>> 102:   unsigned size() const { return _table.table_size(); };
>>> 103: 
>>> 104:   bool add_oop_with_size(oop obj, int size);
>> 
>> Type for `size` should be `jlong`, right? As `typedef ResizeableResourceHashtable` indicates.
>> Let's fix this problem also in `TsanOopMap::add_oop_with_size`.  According to https://github.com/microsoft/compiler-rt/blob/master/test/tsan/java.h, the tsan callbacks take `unsigned long` for size, so `jlong` should be fine.
>> 
>> (We have run into issues with overflowing size field for large objects (>4G), related to JVMTI SampledObjectAlloc. So this is a real bug that could manifest with large objects.)
>
>> Type for size should be jlong, right? As typedef ResizeableResourceHashtable indicates.
> 
> The table size is `unsigned`, see https://github.com/openjdk/jdk21u-dev/blob/570a6bd7df8062ac168f39f368303feaab056978/src/hotspot/share/utilities/resourceHash.hpp#L124 and https://github.com/openjdk/jdk21u-dev/blob/570a6bd7df8062ac168f39f368303feaab056978/src/hotspot/share/utilities/resizeableResourceHash.hpp#L38. 
> 
> The specifically referred type (as `jlong` in your comment) in `typedef ResizeableResourceHashtable` is the type for entry `value`. That's not the table size type. Thank for flagging it though. It's a bug that I missed during code cleanup iterations. I originally tried to use oop as the entry value, hence using `jlong` was appropriate. I ended up using oop size as the entry value instead, but missed to change the value type accordingly. I've fixed to use `size_t` instead.
> 
>> Let's fix this problem also in TsanOopMap::add_oop_with_size. According to https://github.com/microsoft/compiler-rt/blob/master/test/tsan/java.h, the tsan callbacks take unsigned long for size, so jlong should be fine.
> 
> Are you referring to the `size` arg passed in `TsanOopMap::add_oop_with_size(oopDesc *addr, int size)`? That's the oop size, and not the table size. 
> 
> I've also changed the arg type to `size_t` in add_oop_with_size() functions. Note, the caller `SharedRuntime::tsan_track_obj_with_size` still passes the `size` arg as `int`. We can leave that for now and cleanup later.

I'm referring to the argument `int size` passed to `TsanOopMapTable::add_oop_with_size` and `TsanOopMap::add_oop_with_size`. Yes, `size_t` also works.

Thanks for noticing `SharedRuntime::tsan_track_obj_with_size`, we can fix it separately.

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

PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1690368455
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1690377305
PR Review Comment: https://git.openjdk.org/tsan/pull/19#discussion_r1690363507