8297718: Make NMT free:ing protocol more granular #11390

jdksjolen · 2022-11-28T12:38:38Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8297718: Make NMT free:ing protocol more granular

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to e704c024
Gerard Ziemski (@gerard-ziemski - Committer) ⚠️ Review applies to e704c024

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11390/head:pull/11390
$ git checkout pull/11390

Update a local copy of the PR:
$ git checkout pull/11390
$ git pull https://git.openjdk.org/jdk pull/11390/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11390

View PR using the GUI difftool:
$ git pr show -t 11390

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11390.diff

…age()

bridgekeeper · 2022-11-28T12:39:48Z

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-11-28T12:44:50Z

@jdksjolen The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-11-28T12:48:37Z

Webrevs

tstuefe

Mostly good. Mostly style nits.

We now handle correctly a very rare condition that would lead to misaccounting if a realloc failure occurred and the old block was accounted under a different MEMFLAGs. The price is a very slight premium we pay now because we dive down into NMT twice, once to check block integrity, and once to de-account. But since reallocs are not that hot, this should not be noticeable.

Can you please add a small gtest to test that MallocHeader::mark_block_as_dead is fully reversible with mark_block_as_alive, and that the block passes integrity checks afterwards?

tstuefe · 2022-11-28T13:10:09Z

src/hotspot/share/services/mallocHeader.hpp

@@ -115,15 +122,20 @@ class MallocHeader {
  void set_footer(uint16_t v)       { footer_address()[0] = v >> 8; footer_address()[1] = (uint8_t)v; }

 public:
-
+  MallocHeader(const MallocHeader&) = default;


This makes me a bit nervous, where do we copy malloc headers?

Thanks for the catch! It's not necessary, I replaced it with NONCOPYABLE(MallocHeader).

tstuefe · 2022-11-28T13:18:49Z

src/hotspot/share/services/memTracker.hpp

+  }
+  static inline void record_free(FreePackage free_package) {
+    assert(enabled(), "NMT must be enabled");
+    MallocTracker::record_free(free_package);


Please leave the enabled() checks down in MemTracker. You rob it of its purpose in life :) Seriously, should we want to revise that, we should do it for all MemTracker methods.

tstuefe · 2022-11-28T13:34:11Z

src/hotspot/share/services/mallocHeader.hpp

+  const size_t size;
+  const MEMFLAGS flags;
+  const uint32_t mst_marker;
+};


Could this be nested into MallocHeader, and renamed to something like "FreeInfo"?

tstuefe · 2022-11-28T13:35:31Z

src/hotspot/share/services/mallocHeader.hpp

  inline void mark_block_as_dead();
+  inline void mark_block_as_alive();


Proposal: name this "revive" and assert for dead markers. So far this is a one-trick pony used in realloc(), we don't really want a general way to mark dead headers as life.

tstuefe · 2022-11-28T13:36:05Z

src/hotspot/share/services/mallocHeader.hpp

  inline MallocHeader(size_t size, MEMFLAGS flags, uint32_t mst_marker);

  inline size_t   size()  const { return _size; }
  inline MEMFLAGS flags() const { return (MEMFLAGS)_flags; }
  inline uint32_t mst_marker() const { return _mst_marker; }
  bool get_stack(NativeCallStack& stack) const;

+  // Return the necessary data to record the block this belongs to as freed


Proposal: ".. to deaccount block with NMT" (since "record as freed" is ambivalent and could refer to different things, e.g. header marks.

tstuefe · 2022-11-28T13:38:00Z

src/hotspot/share/services/mallocTracker.cpp

-  if (MemTracker::tracking_level() == NMT_detail) {
-    MallocSiteTable::deallocation_at(header->size(), header->mst_marker());
-  }
+  record_free(header->free_package());


Proposal: name this "deaccount(x)", since that's what it does. Then you can keep the original name for record_free().

tstuefe · 2022-11-28T13:39:26Z

src/hotspot/share/services/mallocTracker.hpp

+  // Returns the outer pointer to this block.
+  static void* record_free_block(void* memblock);
+  // Record a free.
+  static void record_free(FreePackage free_package);


Can you expand on the comment a bit?

Given a block returned by os::malloc() or os::realloc(), de-account block from NMT, mark its header as dead and return pointer to header..

Given the free info from a block, de-account block from NMT..

tstuefe · 2022-11-28T13:42:38Z

src/hotspot/share/runtime/os.cpp

+    MallocHeader* header = MallocTracker::malloc_header(memblock);
+    header->assert_block_integrity(); // Assert block hasn't been tampered with.
+    const FreePackage free_package = header->free_package();
+    header->mark_block_as_dead();


Please revive the old comment, suitably adapted. It should still mostly apply, but please mention that de-accounting happens delayed since realloc may fail.

tstuefe · 2022-11-28T13:50:09Z

src/hotspot/share/services/mallocHeader.inline.hpp

+  NOT_LP64(_alt_canary = _header_alt_canary_life_mark);
+  set_footer(_footer_canary_life_mark);
+}
+
 inline void MallocHeader::mark_block_as_dead() {


Can you please add a comment to mark_block_as_dead() that its effects should be fully reversible with mark_block_as_alive (or "revive()", if you take my suggestion).

jdksjolen · 2022-12-01T14:06:53Z

Thank you for the input! I've gone through each comment and fixed the code according to them. I'm far happier with the naming of some of these things now :-).

tstuefe

Looks good to me! Thank you for taking my suggestions.

Cheers, Thomas

openjdk · 2022-12-01T16:39:45Z

@jdksjolen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8297718: Make NMT free:ing protocol more granular

Reviewed-by: stuefe, gziemski

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 282 new commits pushed to the master branch:

073897c: 8294588: Auto vectorize half precision floating point conversion APIs
46cd457: 8298341: Ensure heap growth in TestNativeMemoryUsageEvents.java
1166c8e: 8296896: Change virtual Thread.yield to use external submit
5175965: 8298323: trivial typo in JOptionPane.OK_OPTION
d5cf18e: 8296198: JFileChooser throws InternalError java.lang.InternalError with Windows shortcuts
74f346b: 8298075: RISC-V: Implement post-call NOPs
3aa4070: 8294047: HttpResponseInputStream swallows interrupts
af8fb7e: 8282578: AIOOBE in javax.sound.sampled.Clip
8b69a2e: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
8a9911e: 8295803: Console should be usable in jshell and other environments
... and 272 more: https://git.openjdk.org/jdk/compare/42c2037429a8ee6f683bbbc99fb48c540519524c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

gerard-ziemski · 2022-12-06T20:08:31Z

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?

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

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?

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.

gerard-ziemski · 2022-12-06T20:10:58Z

I had to dig into the NMT code to understand the change and this is my understanding:

OLD:

  MemTracker::record_free(memblock);
    MallocTracker::record_free(memblock);
      header->assert_block_integrity();
      MallocMemorySummary::record_free(header->size(), header->flags());
        as_snapshot()->by_type(flag)->record_free(size);
        as_snapshot()->_all_mallocs.deallocate(size);
      header->mark_block_as_dead();
        _canary = _header_canary_dead_mark;
         NOT_LP64(_alt_canary = _header_alt_canary_dead_mark);
         set_footer(_footer_canary_dead_mark);
  if FAILED(realloc):
    MemTracker::record_malloc(old_outer_ptr, old_size, memflags, stack);
      MallocTracker::record_malloc(mem_base, size, flag, stack);
        MallocMemorySummary::record_malloc(size, flags);
          as_snapshot()->by_type(flag)->record_malloc(size);
          as_snapshot()->_all_mallocs.allocate(size);
    return NULL

NEW:

  MallocHeader* const header = MallocTracker::malloc_header(memblock);
  header->assert_block_integrity();
  const MallocHeader::FreeInfo free_info = header->free_info();
  header->mark_block_as_dead();
    _canary = _header_canary_dead_mark;
    NOT_LP64(_alt_canary = _header_alt_canary_dead_mark);
    set_footer(_footer_canary_dead_mark);
  if FAILED(realloc):
    header->revive();
      _canary = _header_canary_life_mark;
      NOT_LP64(_alt_canary = _header_alt_canary_life_mark);
      set_footer(_footer_canary_life_mark);
    return NULL
  ELSE(realloc):
    MemTracker::deaccount(free_info);
      MallocMemorySummary::record_free(free_info.size, free_info.flags);
        as_snapshot()->by_type(flag)->record_free(size);
        as_snapshot()->_all_mallocs.deallocate(size);

So basically you are re-implementing MemTracker::record_free() without MallocMemorySummary::record_free(), which we will eventually call if realloc() succeeds.

gerard-ziemski · 2022-12-06T20:20:33Z

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.

tstuefe · 2022-12-07T10:40:50Z

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.

tstuefe · 2022-12-07T10:58:41Z

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

Matter of taste. I dislike this kind of coding, even though it is common in hotspot, where you have a compound function and a bunch of flags (often uncommented) fine-controlling its behavior. You then end up having to read the code to understand its behavior exactly.

I like it better to have simple prototypes, simple building blocks and compound actions, and if needed, expose more granular actions with equally simple prototypes. Like here, we have MemTracker::record_free - a compound action that

check NMT enabled
resolve malloc header
deaccount
mark header as dead
return header pointer

and then, we have "deaccount" and "mark header as dead" exposed as more granular action that can be used where you need finer control.

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

There should be little duplication now as well.

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.

You lost me here. Where would that be? Note that in os::realloc(), we are in the code path for NMT enabled already.

tstuefe

Good.

tstuefe · 2022-12-07T11:01:52Z

src/hotspot/share/runtime/os.cpp


    // the real realloc
-    ALLOW_C_FUNCTION(::realloc, void* const new_outer_ptr = ::realloc(old_outer_ptr, new_outer_size);)
+    ALLOW_C_FUNCTION(::realloc, void* const new_outer_ptr = ::realloc((void*)header, new_outer_size);)


The cast is not necessary.

tstuefe · 2022-12-07T11:04:52Z

src/hotspot/share/services/memTracker.hpp

@@ -106,10 +106,14 @@ class MemTracker : AllStatic {
  static inline void* record_free(void* memblock) {
    // Never turned on
    assert(memblock != NULL, "caller should handle NULL");
-    if (!enabled()) {
+    if(!enabled()) {


tstuefe · 2022-12-07T11:06:05Z

src/hotspot/share/services/memTracker.hpp

+    return MallocTracker::record_free_block(memblock);
+  }
+  static inline void deaccount(MallocHeader::FreeInfo free_info) {
+    assert(enabled(), "NMT must be enabled");


breaks protocol for MemTracker (checks enabled) but I think this is here okay. To get a free_info in the first place, we must have had NMT enabled.

gerard-ziemski · 2022-12-07T16:22:49Z

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.
You lost me here. Where would that be? Note that in os::realloc(), we are in the code path for NMT enabled already.

Ah I see it now, it was hiding in the unexpanded portion of the review code - I just needed to click the UI arrow up to reveal it.

gerard-ziemski · 2022-12-07T16:41:36Z

Matter of taste. I dislike this kind of coding, even though it is common in hotspot, where you have a compound function and a bunch of flags (often uncommented) fine-controlling its behavior. You then end up having to read the code to understand its behavior exactly.

Currently we have gone from:

  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);

to:

  MallocHeader* header = MallocTracker::malloc_header(memblock);
  header->assert_block_integrity(); // Assert block hasn't been tampered with.
  const MallocHeader::FreeInfo free_info = header->free_info();
  header->mark_block_as_dead();

But the change could have been just from:

  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);

to:

  MallocHeader* header = MemTracker::record_free(memblock, false);
  const MallocHeader::FreeInfo free_info = header->free_info();

Where header related APIs are nicely encapsulated and hidden away.

Does realloc() really need to know about and do all the header APIs? Before this change realloc() only dealt with MemTracker, but now we have to deal with MallocHeader APIs as well.

gerard-ziemski · 2022-12-07T16:52:09Z

BTW I filed NMT: should realloc() API take flags as arguments? to have a chance to consider whether it is a good idea for realloc to be able to change the MEMFLAGS.

tstuefe · 2022-12-07T17:04:17Z

Matter of taste. I dislike this kind of coding, even though it is common in hotspot, where you have a compound function and a bunch of flags (often uncommented) fine-controlling its behavior. You then end up having to read the code to understand its behavior exactly.

Currently we have gone from:
  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);
to:
  MallocHeader* header = MallocTracker::malloc_header(memblock);
  header->assert_block_integrity(); // Assert block hasn't been tampered with.
  const MallocHeader::FreeInfo free_info = header->free_info();
  header->mark_block_as_dead();
But the change could have been just from:
  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);
to:
  MallocHeader* header = MemTracker::record_free(memblock, false);
  const MallocHeader::FreeInfo free_info = header->free_info();
Where header related APIs are nicely encapsulated and hidden away.

As I wrote, it's a matter of taste. Sometimes a bit verbose is a good tradeoff if it makes for easy understanding.

So, operation x does A B C D, but if I pass false, its A B D. Okay, what if you need a different set of operations? Maybe you need just A and D? Give it another parameter? One for every permutation? That is exactly what often happens, and then you end up with long chain of arguments, each one subtly changing behavior, many of them often one-trick-ponies that are only used at one site.

I quickly find this more complex than another line of plain code at the call site would be. You soon arrive at a point where you cannot tell from the call site what will happen. You have to dive into the code of the subroutine. Especially if both call and parameters are badly named and undocumented, which is often the case within hotspot.

gerard-ziemski · 2022-12-07T19:26:52Z

Matter of taste. I dislike this kind of coding, even though it is common in hotspot, where you have a compound function and a bunch of flags (often uncommented) fine-controlling its behavior. You then end up having to read the code to understand its behavior exactly.

Currently we have gone from:
  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);
to:
  MallocHeader* header = MallocTracker::malloc_header(memblock);
  header->assert_block_integrity(); // Assert block hasn't been tampered with.
  const MallocHeader::FreeInfo free_info = header->free_info();
  header->mark_block_as_dead();
But the change could have been just from:
  const size_t old_size = MallocTracker::malloc_header(memblock)->size();
  void* const old_outer_ptr = MemTracker::record_free(memblock);
to:
  MallocHeader* header = MemTracker::record_free(memblock, false);
  const MallocHeader::FreeInfo free_info = header->free_info();
Where header related APIs are nicely encapsulated and hidden away.
As I wrote, it's a matter of taste. Sometimes a bit verbose is a good tradeoff if it makes for easy understanding.

So, operation x does A B C D, but if I pass false, its A B D. Okay, what if you need a different set of operations? Maybe you need just A and D? Give it another parameter? One for every permutation? That is exactly what often happens, and then you end up with long chain of arguments, each one subtly changing behavior, many of them often one-trick-ponies that are only used at one site.

I quickly find this more complex than another line of plain code at the call site would be. You soon arrive at a point where you cannot tell from the call site what will happen. You have to dive into the code of the subroutine. Especially if both call and parameters are badly named and undocumented, which is often the case within hotspot.

I don't disagree with you in principle, though here I'd prefer modifying the MemTracker::record_free() to hide away as much MallocTracker stuff as possible. I made a suggestion on how I would have done it, and explained my logic behind it, but I am OK with the way we have it currently.

gerard-ziemski · 2022-12-07T16:44:29Z

src/hotspot/share/runtime/os.cpp

+      // realloc(3) failed and the block still exists.
+      // We have however marked it as dead, revert this change.
+      header->revive();
+      return nullptr;


This is the very first usage of nullptr instead of NULL, and we now have a mix of both.

Was this a deliberate choice? Should we then switch all NULLs to nullptr in here?

We should try to use nullptr in new code and change from NULL within those methods for consistency, but a broader cleanup should be done as a separate cleanup RFE.

My strategy for the NULL to nullptr conversion is that if I touch the line of code, I change it to nullptr. Changing whole methods lead to a larger diff for reviewers, which can be annoying when looking for the relevant changes.

Introducing inconsistency in surrounding code is not a good thing though. If you think it too distracting to do the entire method then I would suggest don't do it at all and just leave it all for a cleanup RFE.

gerard-ziemski · 2022-12-07T20:12:14Z

Looks good to me, thank you for the fix Johan!

dholmes-ora · 2022-12-08T02:37:11Z

I quickly find this more complex than another line of plain code at the call site would be.

My concern with the latter is the breaking of encapsulation and pollution of the callsite with details it has no business knowing about. To some extent some knowledge has to leak to the callsite to know either which API to call, or which flag parameters to pass, but that should be minimised and as generic as possible.

tstuefe · 2022-12-08T07:40:26Z

I quickly find this more complex than another line of plain code at the call site would be.

My concern with the latter is the breaking of encapsulation and pollution of the callsite with details it has no business knowing about. To some extent some knowledge has to leak to the callsite to know either which API to call, or which flag parameters to pass, but that should be minimised and as generic as possible.

Okay, I see your point (and @gerard-ziemski ). But having to know when to pass false, and what false does, also breaks encapsulation.

A good encapsulation would be breaking MemTracker::record_free into three clearly named APIs then, without demanding knowledge about what they do:

opaque_thingy MemTracker::prepare_free(void* payload);
void MemTracker::commit_free(opaque_thingy);
void MemTracker::rollback_free(opaque_thingy);

jdksjolen · 2022-12-08T09:57:22Z

I quickly find this more complex than another line of plain code at the call site would be.

My concern with the latter is the breaking of encapsulation and pollution of the callsite with details it has no business knowing about. To some extent some knowledge has to leak to the callsite to know either which API to call, or which flag parameters to pass, but that should be minimised and as generic as possible.

Okay, I see your point (and @gerard-ziemski ). But having to know when to pass false, and what false does, also breaks encapsulation.

A good encapsulation would be breaking MemTracker::record_free into three clearly named APIs then, without demanding knowledge about what they do:
opaque_thingy MemTracker::prepare_free(void* payload);
void MemTracker::commit_free(opaque_thingy);
void MemTracker::rollback_free(opaque_thingy);

A general comment on this thread of reasoning: The original ::record_free() required a comment in os::free that explained exactly what it does, anyway. That's not an abstraction. Functions such as os::free() and os::realloc() should be written by devs that are knowledgeable in NMT, so having a granular API for these parts isn't bad. If this was a "user level" usage, then I'd be more prone to hiding these details.

I'm generally against flags to alter behavior, for the reasons Thomas has already expressed.

jdksjolen · 2022-12-08T10:00:41Z

Thanks for the nits, Thomas. I've pushed them.

jdksjolen · 2022-12-08T12:30:32Z

Tests passing, integrating this.

/integrate

openjdk · 2022-12-08T12:33:15Z

Going to push as commit 165dcdd.
Since your change was applied there have been 290 commits pushed to the master branch:

fbe7b00: 8298173: GarbageCollectionNotificationContentTest test failed: no decrease in Eden usage
d8ef60b: 8298272: Clean up ProblemList
9353899: 8298175: JFR: Common timestamp for periodic events
94575d1: 8295116: C2: assert(dead->outcnt() == 0 && !dead->is_top()) failed: node must be dead
49b8622: 8290850: C2: create_new_if_for_predicate() does not clone pinned phi input nodes resulting in a broken graph
2f426cd: 8298375: Bad copyright header in test/jdk/java/lang/Character/Supplementary.java
b9346e1: 8298033: Character.codePoint{At|Before}(char[], int, int) doesn't do JavaDoc-specified check
297bf6a: 8287397: Print top-level exception when snippet fails to read file
073897c: 8294588: Auto vectorize half precision floating point conversion APIs
46cd457: 8298341: Ensure heap growth in TestNativeMemoryUsageEvents.java
... and 280 more: https://git.openjdk.org/jdk/compare/42c2037429a8ee6f683bbbc99fb48c540519524c...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-08T12:33:43Z

@jdksjolen Pushed as commit 165dcdd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jdksjolen added 6 commits November 23, 2022 14:38

Open up protocol

8e9546f

Assert NMT is enabled in MemTracker::record_free()

9100f47

Rename MallocHeader::free_recording_data() to MallocHeader::free_pack…

e3bd781

…age()

Rename record_free to record_free_block

e391f4e

Rename record_free_header() to record_free()

49fee3d

Add MemTracker::record_free()

7ce15cb

openjdk bot added the rfr Pull request is ready for review label Nov 28, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Nov 28, 2022

tstuefe reviewed Nov 28, 2022

View reviewed changes

jdksjolen added 10 commits December 1, 2022 11:21

Rename as revive, delete copy ctr

0e24449

Move FreePackage into MallocHeader and rename

3c1bd91

Rename record_free to deaccount

aff3d6d

Rename record_free_block to record_free

e6808fd

Note reversibility of mark_block_as_dead

d05ea4b

Put back comment, suitably rewritten

09a0acf

Adjust comments

25d1160

Rest of it

9b01ea8

Make MallocHeader NONCOPYABLE

6f980b8

Add test that asserts block integrity after revival

e704c02

tstuefe approved these changes Dec 1, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2022

tstuefe approved these changes Dec 7, 2022

View reviewed changes

gerard-ziemski approved these changes Dec 7, 2022

View reviewed changes

Nits

bf56099

openjdk bot added the integrated Pull request has been integrated label Dec 8, 2022

openjdk bot closed this Dec 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 8, 2022

		inline void mark_block_as_dead();
		inline void mark_block_as_alive();

8297718: Make NMT free:ing protocol more granular #11390

8297718: Make NMT free:ing protocol more granular #11390

Conversation

jdksjolen commented Nov 28, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 28, 2022

openjdk bot commented Nov 28, 2022

mlbridge bot commented Nov 28, 2022 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdksjolen commented Dec 1, 2022

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 1, 2022 • edited Loading

gerard-ziemski commented Dec 6, 2022

gerard-ziemski commented Dec 6, 2022

gerard-ziemski commented Dec 6, 2022

tstuefe commented Dec 7, 2022

tstuefe commented Dec 7, 2022 • edited Loading

tstuefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerard-ziemski commented Dec 7, 2022

gerard-ziemski commented Dec 7, 2022

gerard-ziemski commented Dec 7, 2022

tstuefe commented Dec 7, 2022 • edited Loading

gerard-ziemski commented Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerard-ziemski commented Dec 7, 2022

dholmes-ora commented Dec 8, 2022

tstuefe commented Dec 8, 2022 • edited Loading

jdksjolen commented Dec 8, 2022

jdksjolen commented Dec 8, 2022

jdksjolen commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

jdksjolen commented Nov 28, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 28, 2022 •

edited

Loading

openjdk bot commented Dec 1, 2022 •

edited

Loading

tstuefe commented Dec 7, 2022 •

edited

Loading

tstuefe commented Dec 7, 2022 •

edited

Loading

gerard-ziemski commented Dec 7, 2022 •

edited

Loading

tstuefe commented Dec 8, 2022 •

edited

Loading