RFR (L): 8046148: JEP 158 Unified JVM Logging

[Marcus Larsson]

On 2015-09-11 14:38, Coleen Phillimore wrote:
>
> Marcus,  Thanks for the reply.   Two short comments (see if you can 
> find them).
>
> On 9/11/15 4:25 AM, Marcus Larsson wrote:
>> Hi,
>>
>> On 2015-09-10 22:30, Coleen Phillimore wrote:
>>>
>>> Hi Marcus,
>>>
>>> This code looks well written and documented in terms of appropriate 
>>> comments in the header files.
>>>
>>> I have a few comments:
>>>
>>> 1.  The NEW and strdup_check_oom() and other memory allocation 
>>> functions use a combination of mtInternal and mtOther, neither of 
>>> which is really going to be helpful for finding memory leaks in 
>>> this.   Can you add an mtLogging tag and use that? There is one 
>>> strdup without an NMT tag (somewhere).
>>
>> Ah, nice catch. I'll fix this!
>>
>>>
>>> 2.  The header files end with #endif but no // BLAH to match the 
>>> #ifdef in the header guard.   Can you add all of these?
>>
>> OK.
>>
>>>
>>> 3.  Nice job adding #includes in alphabetical order.  This is the 
>>> only file that they are not:
>>> http://cr.openjdk.java.net/~mlarsson/8046148/webrev.00/src/share/vm/logging/logFileOutput.hpp.html 
>>>
>>
>> Darn it, thought I had them all. Will fix.
>>
>>>
>>> 4. Some classes don't have storage allocation classes.  It looks 
>>> like they should be VALUE_OBJ_CLASS_SPEC.   Eg. LogOutputList, 
>>> LogDecorations, LogTagSet, LogTag (AllStatic),
>>
>> OK.
>>
>>>
>>> 5. In 
>>> http://cr.openjdk.java.net/~mlarsson/8046148/webrev.00/src/share/vm/logging/logTag.hpp.html 
>>> there is unlikely to be a general 'rt' tag since runtime covers a 
>>> broad set of different features in the vm.
>>
>> I'll remove it along with all the other currently unused tags. If 
>> they're needed we can just add them with the change adding logging 
>> for them.
>>
>>>
>>> Around line 51 in this file, can you explain that MaxTags is that 
>>> you can only give so many tags in any given logging call, not that 
>>> we can only define 5 tags total.   Also, giving a description of how 
>>> to specify more than one tag and what it means (union or 
>>> intersection of that logging tag) would be good to have here too.
>>
>> I'll try to clarify this with some comments.
>>
>>>
>>> On line 58, can Count be "TagCount" or something a bit more 
>>> descriptive?
>>
>> All enums introduced follow this pattern, and when they're used they 
>> must be prefixed with the class they belong to, in this case 
>> LogTag::Count. I think this looks good but I can change it if you 
>> want to. If so, do you want me to make similar changes to 
>> LogLevel::Count and LogDecorators::Count as well?
>
> I only saw Count in logTag.cpp so it looked odd.   Qualified, it looks 
> ok.   Can you qualify it in logTag.cpp instead?

Sounds good!

>>
>>>
>>> 6. In log.hpp can this really long line be broken up to something 
>>> reasonable around 100 chars?
>>>
>>> template <LogTagType T0, LogTagType T1 = LogTag::__NO_TAG, 
>>> LogTagType T2 = LogTag::__NO_TAG, LogTagType T3 = LogTag::__NO_TAG, 
>>> LogTagType T4 = LogTag::__NO_TAG, LogTagType GuardTag = 
>>> LogTag::__NO_TAG>
>>> class Log {
>>
>> OK.
>>
>>>
>>> Thank you for the examples on how to use these.   The template macro 
>>> combinations are pretty insane.   We used to not be allowed to use 
>>> templates in Hotspot.
>>
>> They would be simpler if we had C++11's variadic templates. Now we're 
>> sort of emulating this with up to five tags, which makes it a bit messy.
>>
>>>
>>> 7. In general, could you add a blank line between the header guards 
>>> in the file and the #include statements?
>>>
>>> #ifndef SHARE_VM_LOGGING_LOG_HPP
>>> #define SHARE_VM_LOGGING_LOG_HPP
>>> #include "logging/logLevel.hpp"
>>
>> OK.
>>
>>>
>>> 8. In logTag.hpp there is MaxTags = 5 but the log.hpp file 
>>> definitions have only 5 tags.  It seems like MaxTags should be 
>>> specified there instead or at the top level.
>>
>> I really wanted to have it in the Log class, but then to use it one 
>> would have to specify the tag template argument 
>> (Log<LogTag::some_tag>::MaxTags rather than Log::MaxTags). Should I 
>> rename it to LogMaxTags and make it a global constant (in log.hpp) 
>> instead?
>
> How about just adding a comment then.   I see your point.

Alright, I'll do that.

Thanks again,
Marcus

>
> thanks,
> Coleen
>>
>>>
>>> 9. At the end of logTagSet.hpp, I can't guess what this line is:
>>>
>>> template <LogTagType T0, LogTagType T1, LogTagType T2, LogTagType 
>>> T3, LogTagType T4>
>>> LogTagSet LogTagSetMapping<T0, T1, T2, T3, T4>::_tagset(T0, T1, T2, 
>>> T3, T4);
>>>
>>> it looks like a constructor call.  yes it is.  Can you add a comment 
>>> or something for people who's decoding skills with templates isn't 
>>> as strong?   And you have to define the static field _tagset in the 
>>> .hpp file because it's a template.
>>
>> I'll add comments.
>>
>>>
>>> Can you break up the long template line for LogTagSetMapping also?
>>
>> OK.
>>
>>>
>>> This is all I have for now.  This is a much larger change than I 
>>> appreciated in my pre-review, but I still don't see anything bad at 
>>> all.
>>>
>>> Thanks,
>>> Coleen
>>
>> Thank you for reviewing!
>>
>> Regards,
>> Marcus
>>
>>>
>>> On 9/7/15 9:33 AM, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch adding the unified logging 
>>>> framework to hotspot.
>>>>
>>>> JEP:
>>>> https://bugs.openjdk.java.net/browse/JDK-8046148
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~mlarsson/8046148/webrev.00/
>>>>
>>>> See the JEP description for a general overview of the new feature. 
>>>> Below are some notes on the implementation.
>>>>
>>>> The patch adds the new 'share/vm/logging' subdirectory containing 
>>>> the unified logging framework. The main entry point is log.hpp, 
>>>> which contains the necessary macros and definitions to use the 
>>>> framework.
>>>>
>>>> Log tags are defined/listed in logTag.hpp, and are passed as 
>>>> template arguments to the Log class. Every combination of tags used 
>>>> in a log call has a corresponding LogTagSet instance, which keeps a 
>>>> track of all the outputs it should write the log message to (and 
>>>> their levels). Having tags as template arguments allows mapping 
>>>> directly from a set of tags to the LogTagSet instance, which means 
>>>> that the overhead for disabled logging should be low. Currently 
>>>> each log message can be tagged with up to 5 tags, but this can be 
>>>> increased if ever required (and with C++11's variadic templates the 
>>>> limit can be removed completely).
>>>>
>>>> The LogConfiguration class keeps track of configured outputs 
>>>> (stdout, stderr, and possible file outputs). Configuration is done 
>>>> either by command line arguments (-Xlog) or by DCMD. Both methods 
>>>> will in turn use the LogConfiguration class to perform the parsing 
>>>> & configuration. This configuration includes iterating over all 
>>>> LogTagSet instances and updating them accordingly. The 
>>>> LogTagLevelExpression class is used to represent the selection of 
>>>> tags and levels for a given configuration request (the 
>>>> "what"-expression).
>>>>
>>>> The LogDecorators class contains a selection of decorators. 
>>>> Instances of this class is kept in LogTagSet to track what 
>>>> decorators to use (this is the union of all decorators used by its 
>>>> outputs). Each log call will create a LogDecorations instance 
>>>> (note: different classes), which will contain the actual decoration 
>>>> strings for the log message. These decorations are used for each 
>>>> output the tagset is set to log on, and are then discarded.
>>>>
>>>> The LogPrefix class allows messages of specific sets of tags to be 
>>>> prefixed. The prefix should supply a printf-style format with 
>>>> argument. (This allows GC logging to prefix messages of certain 
>>>> tagsets with GCId.) Prefixes are implemented using template 
>>>> specializations based on the specified tags, with the 
>>>> general/unspecialized case giving an empty prefix.
>>>>
>>>> The LogOutput class defines the interface for all types of log 
>>>> outputs. LogFileStreamOutput corresponds to FILE* stream based log 
>>>> outputs. LogFileOutput builds on this and adds the file management 
>>>> and log rotation support.
>>>>
>>>> A simple jtreg test is included in this patch. Additional tests 
>>>> will be added at a later stage.
>>>>
>>>> Thanks,
>>>> Marcus
>>>
>>
>