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

[Thomas Stuefe]

On Tue, 6 Dec 2022 20:20:33 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> 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.

Hi @gerard-ziemski,

> Just so I understand the motivation for this fix correctly - the "double registration of a failed os::realloc in NMT" does not actually mess up the memory calculations, correct? We do `record_free()` before the failed `realloc()` so from the memory statistics point of view, it is not an issue?

Yes, the only issue is that if `os::realloc` did happen with a different MEMFLAG, and realloc(3) failed, the old allocation would be erroneously accounted to the new MEMFLAG. It's quite theoretical.

> 
> The flags issue that Thomas is referring makes me want to ask: are we really allowed to `realloc()` using different set of flags? It seems just wrong to me (something to look into later perhaps?).

Arguably yes, but the API allows for this. Not sure if there are valid uses for it.

> 
> So if that's the only issue, then we could have potentially just passed `header->flags()` to the "reviving" `MemTracker::record_malloc()` and be done?

Yes, that would have worked. One would have to take care not to use header->flags() if realloc succeeded because the header could have moved in memory. Other than that, this would have been fine too.

> 
> I do appreciate an attempt to clean this area up though and that we only mark the old memory free, after we know that `realloc()` worked and I do think it's an improvement.

Arguably the API surface is now broader, so more complex; but I can live with the current patch too.

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

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