RFR: 8168722: Unified Logging configuration output needs simplifying

[Marcus Larsson]

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.

> 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.

Updated Webrev:
http://cr.openjdk.java.net/~mlarsson/8168722/webrev.01

Incremental:
http://cr.openjdk.java.net/~mlarsson/8168722/webrev.00-01

Thanks,
Marcus