RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests
Severin Gehwolf
sgehwolf at openjdk.org
Wed Dec 14 13:59:02 UTC 2022
On Wed, 14 Dec 2022 11:06:24 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:
>> Hi!
>>
>> Citing the ticket directly:
>>
>> The function subsystem_file_line_contents does the simple job of parsing lines in a file, but does so in a fairly complex way. This function can be simplified. The function also has a surprising API, as you need to know that in one case the format string needs to take 2 specifiers, instead of 1.
>>
>> Some more context:
>>
>> `subsystem_file_line_contents` either parses files that look like this:
>>
>>
>> one_value
>>
>>
>> Called as: `subsystem_file_line_contents(ctrl, fname, nullptr, "%s")`
>>
>> Or like this:
>>
>>
>> key1 val1
>> key2 val2
>>
>>
>> Called as: `subsystem_file_line_contents(ctrl, fname, "key1", "%s %s")`
>>
>> The API for the key/value case is changed to: `subsystem_file_line_contents(ctrl, fname, "key1", "%s")`. Note: `"%s"`, not `"%s %s"`.
>
> test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 137:
>
>> 135: err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
>> 136: EXPECT_EQ(err, 0);
>> 137: EXPECT_STREQ(s, "1");
>
> We should have a test where the separator isn't a single space, like `\t` instead. See https://github.com/jerboaa/jdk/commit/8ab1009a18a95510504f70d8a02c822083fdffd1 That's when those tests currently fail. Like I said previously it would also be good to have a test similar to how `CgroupV2Subsystem::cpu_period()` uses it. Like
> https://github.com/jerboaa/jdk/commit/4d562f08952e17f9137c0da74a3bb587c8539713
I realize you wanted to include this as a separate RFE, but it doesn't make much sense as those additional case are (to me) the bare minimum to have in order to ensure we don't regress.
-------------
PR: https://git.openjdk.org/jdk/pull/11667
More information about the hotspot-runtime-dev
mailing list