RFR: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers [v3]

[Johan Sjölen]

On Thu, 13 Jun 2024 10:20:50 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 52:
>> 
>>> 50:     char *subsystem_path() { return _path; }
>>> 51: };
>>> 52: 
>> 
>> Drive by comment. I don't understand the aim behind the complexity, but multiple inheritance, even with pure abstract side classes, is rarely a good idea. It is also against the hotspot style guide:
>> 
>> 
>> Restrict inheritance to the "is-a" case; use composition rather than
>> non-is-a related inheritance.
>> 
>> Avoid multiple inheritance.  Never use virtual inheritance.
>> 
>> 
>> Most cases I have seen of multiple inheritance are driven by the wish to reuse code, but the added complexity and confusion (e.g. typical C++ IDEs cannot follow call chains) makes life for maintainers actually harder. Composition is usually a better way to do things.
>
> This kind of multiple-inheritance isn't a big deal, it's the same as having multiple `implements` classes in Java. @jerboaa , is it possible to avoid the multiple inheritance by stuffing a `CgroupV1Controller` into the `CgroupV1MemoryController` and so on? Or at least *only* inheriting from classes which are abstract and **only** exposes a set of `virtual = 0;` type methods.
> 
> Basically this:
> 
> ```c++
> class CgroupV1MemoryController {
>   CgroupV1Controller _parent;
> };

Reading this code a bit more, seems like `CgroupController` just exposes a virtual function `subsystem_path`. But this function is only ever called from within `CgroupController`, and we never seem to want to handle a Cgroup controller in a generic way anyway. We can just make the subsystem_path be an argument to all of the functions which require it in `CgroupController` and make that class `AllStatic` and have no one inherit from it.

I also find the general usage of `malloc` concerning. looking at the class `CgroupV1Controller`, this doesn't have a corresponding destructor even though it seems like it owns the memory which it has pointers to. This can just hold the memory directly, no pointers necessary.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1637989287