RFR: 8168722: Unified Logging configuration output needs simplifying

[harold seigel]

This looks good to me, also.

Harold

On 2/28/2018 9:26 AM, Lois Foltan wrote:
> 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
>