RFR: 8337217: Port VirtualMemoryTracker to use VMATree

[Johan Sjölen]

On Fri, 9 Aug 2024 16:05:00 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

> > > Is there a concrete advantage here for using lambda expression when iterating committed regions? I'm asking because personally I find
> > > ```c++
> > >     while ((committed_rgn = itr.next()) != nullptr) {
> > >       print_committed_rgn(committed_rgn);
> > >     }
> > > ```
> > > 
> > > 
> > >     
> > >       
> > >     
> > > 
> > >       
> > >     
> > > 
> > >     
> > >   
> > > simpler and more compact, hence easier to understand, than
> > > ```c++
> > >     CommittedMemoryRegion cmr;
> > >     VirtualMemoryTrackerWithTree::tree()->visit_committed_regions(reserved_rgn, &cmr, [&](CommittedMemoryRegion* crgn) {
> > >       print_committed_rgn(crgn);
> > >       return true;
> > >     });
> > > ```
> > 
> > 
> > Hi Gerard, I'm the one who prefers passing in a lambda and introduced that style, sorry :-).
> > First off, I think the lambda code should be simplified, it should be:
> > ```c++
> > VMWT::tree().visit_committed_regions_of(*reserved_rgn, [](const CommittedMemoryRegion& crgn) {
> >   print_committed_region(crgn));
> >   return true;
> > });
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > I don't think that's too bad, right? The `return true` is a bit unfortunate. It's for being able to stop early, which I agree that regular iterators do better by just performing a `break;`.
> > I'll go ahead and say one nice thing too: If the body of the lambda is a bit more complicated, then we can look at the capture list (the `[]` above) to see what the lambda can alter in the outside scope. With a while-loop, you really have to read the whole thing.
> > **The reason** that I write these kinds of iterators is that they're much simpler to implement and maintain and, _to me_, STL-style iterators aren't easier to read, it's about the same level of complexity.
> > I'll admit that your style of iterators (which are _not_ STL) is about the same complexity, though I find it unfortunate that we have to write an entire class for each iterator.
> > With the simplifications I made, is this style of iterator acceptable?
> 
> hi Johan,
> 
> Yes, this does look better. I looked at https://www.geeksforgeeks.org/when-to-use-lambda-expressions-instead-of-functions-in-cpp to see when one should consider using lambda and I like one particular scenario - improving readability by implementing the code locally, so one can see exactly what is going on without having to look inside a function. I think this is our use case scenario here.
> 
> If only we didn't need all those `return` statements, I'd have clearly preferred lambda here in fact.

It's tedious to have to write `return true;` if you never `return false;` in the function. I looked into fixing this with some metaprogramming, and I found two C++14 solutions and one C++17 one. Here's the Godbolt: https://godbolt.org/z/xGzoGYhf8

And here's the code, reproduced for posterity:

```c++
#include <iostream>
#include <vector>

// C++14 solution
// Here we take extra runtime cost for the void returning function
#define ENABLE_IF(...) \
  std::enable_if_t<bool(__VA_ARGS__), int> = 0


template <typename F,
          ENABLE_IF(std::is_same<bool, typename std::result_of<F(int &)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
  for (int i = 0; i < arr.size(); i++) {
      if (!fun(arr[i])) {
        return;
      }
  }
}

template<typename F,
         ENABLE_IF(std::is_void<typename std::result_of<F(int&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
  iterate(arr, [&fun](int& i) {
    fun(i);
    return true;
  });
}


// C++17 solution, notice that this compiles two functions: one for when early_return is true and when for when it's not.
/*
template<typename F>
void iterate(std::vector<int>& arr, F fun) {
  using ret_type = decltype(fun(arr[0])); // This doesn't actually execute fun(arr[0]);
  constexpr bool early_return = std::is_same<ret_type, bool>::value;
  for (int i = 0; i < arr.size(); i++) {
    if constexpr (early_return) {
      if (!fun(arr[i])) {
        return;
      }
    } else {
      fun(arr[i]);
    }
  }
}
*/

// C++14 solution
// Here we have to make the two implementations ourselves.
/*
#define ENABLE_IF(...) \
  std::enable_if_t<bool(__VA_ARGS__), int> = 0


template <typename F,
          ENABLE_IF(std::is_same<bool, typename std::result_of<F(int &)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
  for (int i = 0; i < arr.size(); i++) {
      if (!fun(arr[i])) {
        return;
      }
  }
}

template<typename F,
         ENABLE_IF(std::is_void<typename std::result_of<F(int&)>::type>::value)>
void iterate(std::vector<int>& arr, F fun) {
  for (int i = 0; i < arr.size(); i++) {
    fun(arr[i]);
  }
}
*/

int main() {
  std::vector<int> my_array{0,1,2,3};

  auto no_early = [](int& x) {
    std::cout << x << " ";
  };
  auto early = [](int &x) {
    if (x == 2) return false;
    std::cout << x << " ";
    return true;
  };
  iterate(my_array, no_early);
  std::cout << std::endl;
  iterate(my_array, early);
  std::cout << std::endl;
}

> I honestly do not think we should be checking in 2 implementations, unless they are nicely hidden away. I do not like the way we manage them right now with 2 explicit `if` checks, each and every time we need to do something.
> 
> If you could push the 2 implementations into a factory class, so that instead of:
> 
> ```
>    if (is_using_sorted_link_list())
>        VirtualMemoryTracker::snapshot_thread_stacks();
>      if (is_using_tree())
>        VirtualMemoryTrackerWithTree::Instance::snapshot_thread_stacks();
> ```
> 
> we have just:
> 
> ` VirtualMemoryTracker::snapshot_thread_stacks();` and
> 
> ```
>   VirtualMemoryTracker::VirtualMemoryTracker(bool useLinkedList) {
>     if (useLinkedList) {
>       instance = new VirtualMemoryTrackerWithLinkedList()
>     } else {
>       instance = new VirtualMemoryTrackerWithLinkedList()
>     }
>   }
>   
>   VirtualMemoryTracker::snapshot_thread_stacks() {
>     instance.snapshot_thread_stacks();
>   }
> ```

>From reading the code, this might be a bit harder to do than preferred. We'd need some wrapping for the iterators, since they use two different styles. Wrapping the "old" iterators inside of the "new" style is possible of course.

The fork for JDK-24 is on December 5th. That means that we have at most 8 weeks in the testing system to find and fix any bugs that we might have missed after integration. To me, that feels a bit short. Either we wait after the fork to integrate, or we integrate with two implementations and a `java` option for choosing implementation. I'm open to other opinions.

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

PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2309938825
PR Comment: https://git.openjdk.org/jdk/pull/20425#issuecomment-2378890343