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

[Coleen Phillimore]

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

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