RFR: 8302744: Refactor Hotspot container detection code [v2]

[Severin Gehwolf]

On Tue, 7 May 2024 09:52:24 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

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

@jdksjolen @tstuefe Please take another look. I've introduced two simple macros for "checked" reading and the primary reading functions are reading a string or an unsigned long. Please let me know what you think.

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

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