RFR: 8168722: Unified Logging configuration output needs simplifying

[Lois Foltan]

On 2/28/2018 5:04 AM, Marcus Larsson wrote:

> Hi Lois,
>
> Thanks for reviewing, replies inline.
>
>
> On 2018-02-27 20:55, Lois Foltan wrote:
>> On 2/20/2018 3:51 AM, Marcus Larsson wrote:
>>
>>> Hi,
>>>
>>> Please review the following patch to make UL configuration 
>>> descriptions more readable/usable. Instead of explicitly listing all 
>>> tag sets with their corresponding levels (a=info,b=info, ..., 
>>> x=debug, etc), a significantly shorter aggregate is now generated 
>>> and printed instead (all=info,x=debug).
>>>
>>> Summary:
>>> Initially, the configured level for the majority of the log tag sets 
>>> is found and used for the "all=<level>" part of the config string. 
>>> Then, using a greedy iterative algorithm, selections are generated 
>>> and added to the configuration string to cover the descriptions for 
>>> tag sets with levels deviating from the current description. In each 
>>> step, the selection which covers the most deviating tag sets is 
>>> chosen. This is repeated until there are no deviating tag sets 
>>> remaining. For the curious, the problem can be thought of as a 
>>> variation of the set cover problem [0].
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8168722
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8168722/webrev.00
>>>
>>> Tested with hs-tier 1-2, and locally with 
>>> jtreg/{serviceability,runtime,gc}/logging.
>>>
>>> Thanks,
>>> Marcus
>>>
>>> [0]: https://en.wikipedia.org/wiki/Set_cover_problem
>>
>> Hi Marcus,
>> Overall this looks good.  Just a couple of minor comments:
>>
>> - hotspot/share/logging/logOutput.cpp
>> line #43 - please consider making delimiter a char
>> line #102  - can subset_size and depth be const parameters?
>> line #139 - consider not post incrementing subset_size (make a const 
>> parameter), since subset_size is passed by value parameter, the 
>> incrementation won't make a difference after a return from the 
>> current call to generate_all_subsets_of.  I would rather see line 
>> #140 explicitly use "subset_size + 1". That then it makes it obvious 
>> that on the 2nd call to generate_all_subsets_of, subset_size has been 
>> incremented.
>> line #155 - is the variable "before" ever used?  I think it can be 
>> removed.
>
> All great suggestions. Fixed!
>
>> line #201 & 202, why the bump by 2?
>
> The check ensures there's enough room for both the exact and wildcard 
> selections, instead of doing two separate checks in a row. I added a 
> comment to clarify.
>
>> line #214 - can on_level be const?  It doesn't seem to be assigned 
>> into within update_config_string().
>
> Indeed, fixed.
>
>> line #239 - why is selections_cap set to 64?
>
> It's just a guesstimate on how many selections will be generated. I 
> made it depend on the MaxSubsets constant and increased it a bit. Also 
> added a comment to clarify that it's the initial cap.
Thanks for the above fixes and additional comments.

>
>> line#259 - best_selection is defined to be a constant array but at 
>> line #286 you basically reassign the whole array, that looks fishy.
>
> It's not an array, it's a pointer to the currently best selection found.
Oops, I read that wrong.  Thanks for clarifying.

>
> Updated Webrev:
> http://cr.openjdk.java.net/~mlarsson/8168722/webrev.01
>
> Incremental:
> http://cr.openjdk.java.net/~mlarsson/8168722/webrev.00-01
Looks good!
Lois

>
> Thanks,
> Marcus