[master] RFR: JVMTI: Don't use object header for marking

[Thomas Stuefe]

On Wed, 23 Mar 2022 19:16:58 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

> JVMTI marks objects in order to track whether or not it has already visited objects during heap walking. This uses the usual GC marking bits in the object header. However, this proves to be confusing and brittle because some GCs also uses those header bits for marking and/or indicating forwarded objects. In particular, it becomes unreliable for Shenandoah GC to distinguish JVMTI marked objects from forwarded objects.
> 
> JVMTI should have no business in marking objects in their header. This change proposes to let JVMTI use its own (temporary) marking bitmap instead. This decouples JVMTI better from GCs.
> 
> Testing:
>  - [x] tier1
>  - [x] tier2
>  - [ ] tier3

Hi Roman,

Trying to understand.

You reserve a bitmap to cover the whole reserved heap space, with one bit per possible object location? So the memory use would be -Xmx / 12, right? (8 bits per byte, 3 bit shift)?

So I calculate ~90MB per GB address space. Am I thinking right?

This may hurt a bit for applications which use jvmti heap walk as OOM analysis tool. But I guess short term its okay.

Will in the course of walking every object be visited, so every bit be set eventually, or would the bitmap be more sparse? If the latter, we may reduce the footprint in the future by making a on-demand-commited bitmap, only committing pages with set bits. Would make clearing faster too.

Do we need a bitmap for the whole reserved range, would a committed range not simpler? But probably difficult to do if multiple committed regions exist.

Oh, but then more motivation in the future to support a sparse bitmap.

---

Comment before RestoreMarksClosure needs massaging.

Cheers, Thomas

While thinking this through more fully, if the bitmap is used sparsely it won't hurt that much, since we don't touch a lot of memory, even if committed. It only hurts if you hit the commit limit.

Just thinking, you probably don't need to clear the bitmap at all. os::commit_memory() remaps the space, so its a fresh mmap segment, and anonymous mmap segments are zero-initialized.

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

> 1368: 
> 1369: void ObjectMarker::initialize(MemRegion heap_region) {
> 1370:   new (&_mark_bit_map) MarkBitMap();

This construct is a bit awkward. Would be cleaner to have the bitmap just newd and deleted, or even build a compound object containing the bitmap and its ReservedSpace.

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

> 1371:   size_t bitmap_size = MarkBitMap::compute_size(heap_region.byte_size());
> 1372:   ReservedSpace bitmap(bitmap_size);
> 1373:   _bitmap_region = MemRegion((HeapWord*) bitmap.base(), bitmap.size() / HeapWordSize);

I'd use BitsPerWord, not HeapWordSize. Potential conflict: MemRegion uses word size in HeapWord, but Bitmap ultimately expects a memory range in bm_word_t, and uses BitsPerWord internally.

Its a bit of a brain teaser. I would feel better if MarkBitMap::initialize() would assert that the bitmap region covers what it thinks it should cover. Currently it does not check the bitmap region size at all.

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

> 1386:   if (!os::commit_memory((char*)_bitmap_region.start(), _bitmap_region.byte_size(), false)) {
> 1387:     vm_exit_out_of_memory(_bitmap_region.byte_size(), OOM_MALLOC_ERROR,
> 1388:                           "Could not commit native memory for auxiliary marking bitmap for JVMTI object marking");

Use os::commit_memory_or_exit ?

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

> 1395:   if (!os::uncommit_memory((char*)_bitmap_region.start(), _bitmap_region.byte_size())) {
> 1396:     log_warning(gc)("Could not uncommit native memory for auxiliary marking bitmap for JVMTI object marking");
> 1397:   }

Clear bitmap?

src/hotspot/share/prims/jvmtiTagMap.hpp line 44:

> 42:   static MemRegion  _bitmap_region;
> 43: public:
> 44:   static void initialize(MemRegion heap_region);

So, initialize() runs at VM startup, init() runs when the heap walk starts, if ever?

I'd rename init() to prepare_heapwalk or something similar. initialize vs init is not clear.

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

PR: https://git.openjdk.java.net/lilliput/pull/45