RFR: 8168722: Unified Logging configuration output needs simplifying

[Marcus Larsson]

Thanks for reviewing Harold!

Marcus


On 2018-02-28 19:58, harold seigel wrote:
> 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
>>
>