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

[Marcus Larsson]

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?

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

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