RFR: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers [v3]
Johan Sjölen
jsjolen at openjdk.org
Thu Jun 13 10:42:16 UTC 2024
On Wed, 12 Jun 2024 19:13:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 30 commits:
>>
>> - Review feedback
>> - Clean up
>> - Appropriately handle version specific printing
>> - Fix inheritance hierarchy
>> - Fixup after merge
>> - Merge branch 'master' into jdk-8331560-cgroup-controller-delegation
>> - Resolve ambiguity with static_cast
>> - Remove limit_from_str from util class
>> - Merge branch 'jdk-8302744-cleanup-getcontainer-info' into jdk-8331560-cgroup-controller-delegation
>> - Add proper comments for parsing utility functions
>> - ... and 20 more: https://git.openjdk.org/jdk/compare/9b64ece5...e177a086
>
> 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;
};
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1637968872
More information about the hotspot-runtime-dev
mailing list