RFR: 8302744: Refactor Hotspot container detection code

[Johan Sjölen]

On Mon, 6 May 2024 10:51:06 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Please review this container detection code refactoring in hotspot. The main point of this is to
>> 
>> - get rid of the `GET_CONTAINER_INFO` macros which hide too many things under the hood
>> - prevent refactoring of the code (since `GET_CONTAINER_INFO` macros short-return and are therefore not portable; at least not without some risk)
>> - make the code easier to understand
>> - allow for better testing via `gtest`
>> - separate multi-line parsing from single line parsing for clarity.
>> 
>> Testing:
>> - [x] GHA
>> - [x] `gtest:cgroupTest` tests
>> - [x] Container tests on Linux with cgroup v1 (legacy) and cgroup v2. All pass.
>> 
>> Thoughts?
>
> Hi Severin,
> 
> I've gone through a bit more of the code now.
> 
> I'm really wondering if simply doing the macro expansion is the right way to go here. So many of these do the same thing, isn't there a better pattern? I'm thinking something like this:
> 
> 1. Change the calling convention to taking an out-parameter reference and return an int for error
> 
> ```c++
> int CgroupV1MemoryController::uses_mem_hierarchy(jlong& use_hierarchy) {
>   jlong temp;
>   int err = cg_file_contents_ctrl(this, "/memory.use_hierarchy", JLONG_FORMAT, &temp);
>   if (err != 0) {
>     log_trace(os, container)("Use Hierarchy is: %d", OSCONTAINER_ERROR);
>     return err;
>   }
>   use_hierarchy = temp;
>   log_trace(os, container)("Use Hierarchy is: " JLONG_FORMAT, use_hierarchy);
>   return 0;
> }
> 
> 
> 2. Abstract out the message such that it is obvious when it failed and not:
> 
> ```c++
> int CgroupV1MemoryController::uses_mem_hierarchy(jlong& use_hierarchy) {
>   const char* str = "Use Hierarchy";
>   jlong temp;
>   int err = cg_file_contents_ctrl(this, "/memory.use_hierarchy", JLONG_FORMAT, &temp);
>   if (err != 0) {
>     log_trace(os, container)("%s failed: %d", str, OSCONTAINER_ERROR);
>     return err;
>   }
>   log_trace(os, container)("%s is: " JLONG_FORMAT, str, use_hierarchy);
>   return 0;
> }
> 
> 
> 3. Port this pattern over to other similar methods and introduce a common macro for creating them. Here's some make believe code
> 
> ```c++
> #define V1_SIMPLE_ACCESSORS_LIST \
> SIMPLE_ACCESSOR(julong, kernel_memory_usage_in_bytes,  _memory->controller(), "/memory.kmem.limit_in_bytes", "Kernel Memory Limit", JULONG_FORMAT, kmem_limit) \
> SIMPLE_ACCESSOR(jlong, uses_mem_hierarchy, this,  "/memory.use_hierarchy", "Use Hierarchy", JLONG_FORMAT, use_hierarchy) \
> SIMPLE_ACCESSOR(jlong, read_mem_swappiness, _memory->controller(), "/memory.swappiness", "Swappiness", JULONG_FORMAT, swappiness)
> 
> #define SIMPLE_ACCESSOR(ret_type, name, accessor, filename, logformat, logformat2, retname) \
> int CgroupV1Subsystem::name(ret_type& retname) { \
>   ret_type temp; \
> int err = cg_file_contents_ctrl(accessor, filename, logformat2, &temp); \
> if (err != 0) { log_debug(os)("%s: failed %d" logformat, err); return err; } \
>  log_debug(os)("%s: failed " logformat2 logformat, temp); \
>  retname = temp; \
>  return 0; }
> V1_SIMPLE_ACCESSORS_LIST
> #undef SIMPLE_ACCESSOR
> 
> 
> Which generates the following code (checked using `gcc -E` and formatted using `clang-format`)
> 
> ```c++
> int CgroupV1Subsystem::kernel_memory_usage_in_bytes(julong &kmem_limit) {
>   julong temp;
>   int err = cg...

> Thanks all for the reviews! All good suggestions, though I'm not sure I like the macro approach @jdksjolen suggested. It still comes at the cost of harder to read code. I'll do further exploration and will hopefully come back with a better approach.

Yeah, the X-macro approach may not be the best either. This is clearly a trade off between ease of reading one function, and the ease of making many extremely similar functions with few lines of code.

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

PR Comment: https://git.openjdk.org/jdk/pull/19060#issuecomment-2097904817