RFR: 8283710: JVMTI: Use BitSet for object marking [v5]

[Thomas Stuefe]

On Thu, 7 Apr 2022 17:20:41 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> JVMTI heap walking marks objects in order to track which have been visited already. In order to do that, it uses bits in the object header. Those are the same bits that are also used by some GCs to mark objects (the lowest two bits, also used by locking code). Some GCs also use the bits in order to indicate 'forwarded' objects, where the upper bits of the header represent the forward-pointer. In the case of Shenandoah, it's even more problematic because this happens concurrently, even while JVMTI heap walks can intercept. So far we carefully worked around that problem, but it becomes very problematic in Lilliput, where accesses to the Klass* also requires to decode the header, and figure out what bits means what.
>> 
>> In addition to that, marking objects in their header requires that the original header gets saved and restored. We only do that for 'interesting' headers, that is headers that have a stack-lock, monitor or hash-code. All other headers are reset to their default value. This means we are losing object's GC age. This is not catastrophic, but nontheless interferes with GC. 
>> 
>> JFR already has a datastructure called BitSet to support object marking without messing with object's headers. We can use that in JVMTI too.
>> 
>> Testing:
>>  - [x] tier1
>>  - [x] tier2
>>  - [x] tier3
>>  - [x] serviceability/jvmti
>>  - [x] vmTestbase/nsk/jvmti
>
> Roman Kennke has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Add comment describing ObjectBitSet
>  - Refactor JVMTI usage of ObjectBitSet
>  - Typedef ObjectBitSet<mtTracing> to JFRBitSet in JFR code
>  - Rename BitSet to ObjectBitSet

Looks better :) Placed in the VMOp this is less confusing. Also, less code.

Small remarks inline, feel free to ignore them. We are entering nitpicking territory here.

Cheers, Thomas

src/hotspot/share/jfr/leakprofiler/chains/bfsClosure.hpp line 37:

> 35: class ObjectBitSet;
> 36: 
> 37: typedef ObjectBitSet<mtTracing> JFRBitSet;

Can you include memory/allocation.hpp for the MEMFLAGS? iterator.hpp already does, but better to be include-clean

src/hotspot/share/prims/jvmtiTagMap.cpp line 2252:

> 2250:   GrowableArray<oop>* _visit_stack;                 // the visit stack
> 2251: 
> 2252:   JVMTIBitSet* _bitset;

Small nit: You may consider making this a simple member, then you can get rid of the dynamic allocation and deallocation. Just feed the address of the member to CallbackInvoker. 

If you are worried about stack size, Bitset is not that big, 100ish bytes or so. Should still be fine to put the VMOp on the stack.

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

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7964