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

[Thomas Stuefe]

On Mon, 4 Apr 2022 17:51:28 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 one additional commit since the last revision:
> 
>   Templatize BitSet for MEMFLAGS

Hi @coleenp and @rkennke,

Since the JVMTI heap walk usually walks the whole heap, the bitmap may not be as sparse as we think. It would materialize a lot - possibly all - of its fragments. The increased memory consumption during JVMTI heap walk may mess with JVMTI agents using heapwalking for analysis in OOM situations.

But I'm not against this. It is clearly more maintainable. A lot clearer. And if it helps lilliput, we save a lot more memory than we pay here.

---

About this PR:

You moved Bitset to general-purpose land. Now its a general purpose class, and should be more versatile and better documented. A comment would be good, at least for the structure itself. Proposal (feel free to modify/extend):


BitSet is a sparse bitmap describing a memory range. It holds one bit per xxx-aligned
address. Its underlying backing memory is allocated on-demand only, in yyy-sized
fragments. Fragments are never deleted. The underlying memory is allocated from C-Heap.


I would also rename the function, since it is very unclear what the difference is between BitMap and BitSet. E.g. "AddressMap" "SparseAddressMap", or similar.

fragment size xxx and alignment yyy would ideally be configurable at compile time. At the moment, it feels very "heapish". It uses oop. It uses LogMinObjAlignmentInBytes. Ideally we would not include oop.hpp, and not know about oops.

Especially the dependency on LogMinObjAlignmentInBytes is not optimal: that is a runtime variable depending on `-XX:ObjectAlignmentInBytes`. It would be good if that could be a compile time constant. Right now, IIUC, `BitSet<F>::addr_to_bit()` will always load the alignment from memory.

As a general purpose structure, some gtests would be nice. Maybe in some future RFE.

---

A general thought, maybe for a future RFE:

We now have BitMap and BitSet, which do almost the same thing. Instead of having a new class BitSet, whose job is to be sparse, we could give BitMap the ability to be sparse. We'd save code, reuse the rich BitMap interface and get all the test code for free.

One way to do that would be to make BitMap responsible for committing its underlying backing memory, in chunks, on-demand only. To do that BitMap would need a secondary map - a commit map - to remember which fragments of its backing memory are committed. Commit map would have a bit per 64M fragment.

As a sketch:


class BitMap {

   // bitmap holding one bit per 64M fragment of the underlying bitmap backing memory
   BitMap _commit_map;
   
   inline void set_bit(idx_t bit) {
   	// - find out which fragment the bit lives in
   	// - if (commitmap[fragmentid] == 0) { commit_fragment(); commitmap[fragmentid]=1; }
   	// - set bit
   }
   
   // same for clear_bit()
   
   bool at(idx_t index) const {
   	// - find out which fragment the bit lives in
   	// - if (commitmap[fragmentid] == 0) { return false; } // bit has never been set, is therefore zero
   	// - else return the actual bit
   }

}


The sparse-handling part would have to be switchable at compile time so that we won't pay for it in non-sparse BitMap.

I believe this solution could be simpler than a new separate utility class, and be more versatile. Would be interesting to know @kimbarrett 's opinion. 

But this can be done of course in a separate RFE. If at all.


Cheers, Thomas

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

> 40:   EdgeQueue* _edge_queue;
> 41:   EdgeStore* _edge_store;
> 42:   BitSet<mtTracing>* _mark_bits;

It feels weird to have an implementation detail like "under which NMT category is the bitmap allocated" in the interface of all functions now. I don't have a better idea for now, but how about we at least typedef this thing in JFR space somewhere? E.g. `typedef BitSet<mtTracing> JFRBitSet` ?

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

> 1335: // Stack allocated class to help ensure that ObjectMarker is used
> 1336: // correctly. Constructor initializes ObjectMarker, destructor calls
> 1337: // ObjectMarker's done() function to restore object headers.

This comment is outdated, right?

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

> 1336: // correctly. Constructor initializes ObjectMarker, destructor calls
> 1337: // ObjectMarker's done() function to restore object headers.
> 1338: class ObjectMarker : public StackObj {

Maybe, for a future RFE: Ideally this would live as member in the VMOp, constructed and cleaned up by it. No stack obj nor global pointer needed then. We'd have to fix those few callers that have no access to the calling VMOp though.

src/hotspot/share/utilities/bitset.hpp line 54:

> 52:       }
> 53:     };
> 54: 

We can loose the  "protected" below since we never derive from this class

src/hotspot/share/utilities/bitset.inline.hpp line 94:

> 92: template<MEMFLAGS F>
> 93: inline BitMap::idx_t BitSet<F>::addr_to_bit(uintptr_t addr) const {
> 94:   return (addr & _bitmap_granularity_mask) >> LogMinObjAlignmentInBytes;

See my earlier comment. This is a runtime variable, would be nice to have this as compile time constant.

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

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