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

[Gerard Ziemski]

On Wed, 7 Dec 2022 17:04:17 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> 



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

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

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