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

[Thomas Stuefe]

On Thu, 23 May 2024 14:34:32 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> @jerboaa 
>> 
>> A lot better! 
>> 
>> If you move the sscanf up to `read_(number|numerical_tuple_value)()`, things become even simpler and you can remove the templatization altogether.
>> 
>> Also, at the moment, when you call read_string, three different buffers are involved, and three buffer copies. And one of the buffers is malloc'ed, which is not ideal for a high frequency call. And it forces you to deal with freeing that buffer.
>> 
>> You have:
>> 
>> - `read_number` -> `read_from_file`
>> - `read_numerical_tuple_value` -> `read_from_file`
>> - `read_string` -> `read_from_file`
>> 
>> You can move the sscanf up to the callers:
>> 
>> - Let `read_from_file()` only read the first line from a file and return that. No sscanf. Let it take an output buffer (e.g. `char output[1024]`) as argument.
>> - Let `read_number()` call `read_from_file` with a local buffer, then call sscanf with its `JULONG_FORMAT`
>> - Let `read_numerical_tuple_value()` call `read_from_file` with a local buffer, then call sscanf with the TupleFormat
>> - Let `read_string` call `read_from_file` with a local buffer. No need for sscanf. It can just return the strduped buffer.
>> 
>> No need for templatization of `read_from_file` then.
>> 
>> An extension of that idea would be to not use strdup in `read_string` at all, but to require a caller-provided buffer. Then, we don't have to copy data around, and you could just remove `read_string` altogether, since it adds nothing of value. Callers can call `read_from_file` directly.
>> 
>> Removing the strdup from the read_string path would mean you can also remove all the `os::free` calls in limit_from_str. No need to think about who owns which buffers.
>> 
>> You would introduce local buffers in those functions that call read_string. In those that work on caller-provided buffers, return either the caller-provided buffer or NULL to indicate an error (which is the typical C-style pattern), e.g.:
>> 
>> 
>> 
>> char* CgroupV2Subsystem::pids_max_val(char buffer[1024]) {
>>   CONTAINER_READ_STRING_CHECKED(_unified, "/pids.max", "Maximum number of tasks", buffer);
>>   return buffer;
>> }
>> 
>> jlong CgroupV2Subsystem::pids_max() {
>>   char buffer[1024]
>>   char * pidsmax_str = pids_max_val(buffer);
>>   return limit_from_str(buffer);
>> }
>> 
>> 
>> Yet another way to avoid malloc would be using ResourceAreas. But that introduces dependencies to initialization, and I hesitate having cgroups handling depend on VM initialization.
>
> @tstuefe @jdksjolen Take 3 :) No more templates involved. PTAL when you get a chance. Thanks again for the feedback! Much appreciated.

@jerboaa nice :) Will take a look next week

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

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