RFR: 8297718: Make NMT free:ing protocol more granular [v2]

[Gerard Ziemski]

On Thu, 1 Dec 2022 13:59:25 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> This PR avoids the double registration of a failed `os::realloc` in NMT by doing the NMT free protocol steps inline (asserting block integrity, marking of MallocHeader, and registrering results to statistics). This is done by creating a new struct `FreePackage` which includes all of the necessary data for registrering results.
>> 
>> I also did some light refactoring which I think makes the protocol clearer, these are done piece wise as separate commits for the reviewers' convenience.
>
> Johan Sjölen has updated the pull request incrementally with 10 additional commits since the last revision:
> 
>  - Add test that asserts block integrity after revival
>  - Make MallocHeader NONCOPYABLE
>  - Rest of it
>  - Adjust comments
>  - Put back comment, suitably rewritten
>  - Note reversibility of mark_block_as_dead
>  - Rename record_free_block to record_free
>  - Rename record_free to deaccount
>  - Move FreePackage into MallocHeader and rename
>  - Rename as revive, delete copy ctr

I think we should have reused the original `MemTracker::record_free()` and modified it to take an additional argument, something like:

**OLD:**

static inline void* record_free(void* memblock) {
    // Never turned on
    assert(memblock != NULL, "caller should handle NULL");
    if (!enabled()) {
      return memblock;
    }
    return MallocTracker::record_free(memblock);
}

void* MallocTracker::record_free(void* memblock) {
  assert(MemTracker::enabled(), "Sanity");
  assert(memblock != NULL, "precondition");
  MallocHeader* const header = malloc_header(memblock);
  header->assert_block_integrity();
  MallocMemorySummary::record_free(header->size(), header->flags());
  if (MemTracker::tracking_level() == NMT_detail) {
    MallocSiteTable::deallocation_at(header->size(), header->mst_marker());
  }
  header->mark_block_as_dead();
  return (void*)header;
}


**NEW:**

static inline void* record_free(void* memblock, bool record = true) {
    // Never turned on
    assert(memblock != NULL, "caller should handle NULL");
    if (!enabled()) {
      return memblock;
    }
    return MallocTracker::record_free(memblock, record);
}

void* MallocTracker::record_free(void* memblock, bool record = true) {
  assert(MemTracker::enabled(), "Sanity");
  assert(memblock != NULL, "precondition");
  MallocHeader* const header = malloc_header(memblock);
  header->assert_block_integrity();
  if (record) {
    MallocMemorySummary::record_free(header->size(), header->flags());
    if (MemTracker::tracking_level() == NMT_detail) {
      MallocSiteTable::deallocation_at(header->size(), header->mst_marker());
    }
  }
  header->mark_block_as_dead();
  return (void*)header;
}


This is way we are re-using as much code as possible without needing to duplicate much of it.

But more importantly, we would **NOT** miss this code:


    if (!enabled()) {
      return memblock;
    }


As it is now, we are calling NMT code always, even when NMT is OFF.

You can of course add that check itself back to your re-implementation copy directly in `os::realloc()`, but like I said, we should just modify the original `MemTracker::record_free()` method (and possibly give it a more apt name now that we can skip the record_free portion of it) and re-use the already existing code.

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

PR: https://git.openjdk.org/jdk/pull/11390