RFR: 8142506: Reimplement TraceClassUnloading with Unified Logging.

[Ioi Lam]

On 11/13/15 7:31 AM, Staffan Larsen wrote:
>> On 13 nov. 2015, at 14:57, David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 13/11/2015 11:42 PM, Max Ockner wrote:
>>> David,
>>>
>>> We chose Trace* and Print* options to convert to Unified Logging because
>>> those options do most of the logging.  Trace level logging is basically
>>> the new WizardMode/Verbose. Flags that could once be called either alone
>>> or with WizardMode can now be logged with different levels. For example,
>>>
>>> "-XX:+TraceClassUnloading" will hit code guarded by "if
>>> (TraceClassUnloading)", but not "if (TraceClassUnloading &&
>>> WizardMode)". To see logging contained in the WizardMode statement, you
>>> must specify WizardMode as well.  When This all gets converted to
>>> Unified Logging,  logging that was once guarded by "if
>>> (TraceClassUnloading)" will now be accessible with
>>> "-Xlog:classunload=debug", and logging that was guarded by "if
>>> (TraceClassUnloading && WizardMode)" will be accessible through
>>> "-Xlog:classunload=trace".
>> It is that mapping that I am questioning: why should it not be classunload=info as it is the initial level of logging.
> +1: classunload=info seems the more natural mapping for UL.

Currently, my "classload" patch and Max's "classunload" patch use these 
levels"

[some output] = debug
[more output] = trace

Are you saying we should use the following instead?

[some output] = info
[more output] = debug

Thanks
- Ioi

>
>>> For this particular case, I could also imagine using the Info level for
>>> the majority of the logging to make it accessible with
>>> "-Xlog:classunload", and if we wanted to keep the WizardMode output out
>>> of product, we could either (1) use #ifndef PRODUCT or (2) change to
>>> develop level for those outputs. However, I don't see a problem with
>>> allowing the WizardMode output in product builds.  This output is
>>> currently blocked from product builds by WizardMode, which is a product
>>> only flag. There are no #ifdefs that we'd have to remove, so I don't
>>> expect any kind of performance or size issues with this change.
>> Including non-product code in a product build will affect both size and performance, though to what exact extent we can't say a-priori. A develop flag in a product build is a constant and so code guarded by eg WizardMode can be elided in a product build.
>>
>> Cheers,
>> David
>>
>>> Thanks,
>>> Max
>>>
>>> On 11/13/2015 12:31 AM, David Holmes wrote:
>>>> Hi Max,
>>>>
>>>> I have not looked at the code :) This is more a meta-commentary on the
>>>> general approach to conversion ...
>>>>
>>>> On 12/11/2015 6:23 AM, Max Ockner wrote:
>>>>> Hello,
>>>>> Please review my change.
>>>>>
>>>>> Bug:  https://bugs.openjdk.java.net/browse/JDK-8142506
>>>>> Webrev:  http://cr.openjdk.java.net/~mockner/classunload/
>>>>>
>>>>> Testing:
>>>>> Hotspot jtreg tests with and without "-Xlog:classunload=trace"
>>>>> runthese tests with "-Xlog:classunload=trace"
>>>>> new jtreg test for classunload output.
>>>>>
>>>>> Summary:
>>>>>
>>>>> There are two parts to this fix.
>>>>>
>>>>> (1) Existing logic for TraceClassUnloading has been replaced with
>>>>> commands from Unified Logging.
>>>>>
>>>>> (2) Logging Alias Table. "-XX:+TraceClassUnloading" is now mapped to
>>>>> "-Xlog:classunload=debug" in the new logging alias table in
>>>>> arguments.cpp. Arguments are checked in the beginning of
>>>>> parse_each_vm_init_arg and if an argument (such as
>>>>> -XX:+TraceClassUnloading") is found in the alias table, the optionString
>>>>> is substituted for the value found in the logging alias table.
>>>>>
>>>>> I had to make a few decisions along that might raise questions, so I
>>>>> will knock a few of them out now.
>>>>>
>>>>> Why didn't I use the existing alias table?
>>>>> - Existing alias table only stores the flag name (TraceClassUnloading)
>>>>> and is meant for substituting one "-XX:" argument for another. This does
>>>>> not solve our problem.
>>>>>
>>>>> Why didn't I process the logging aliases later in
>>>>> parse_each_vm_init_arg, next to the rest of the "-XX:" argument
>>>>> processing?
>>>>> - The main processing for "-XX:" options happens after processing of
>>>>> "-Xlog" options. If we want the unified logging alias to be handled, it
>>>>> needs to be substituted in before "-Xlog:" is handled.
>>>>>
>>>>> Why does "-XX:+TraceClassUnloading" get aliased to
>>>>> "-Xlog:classunload=debug" and not "-Xlog:classunload=trace"?
>>>>> - logging statements that contained the develop-only flag "WizardMode"
>>>>> were converted to "-Xlog:classunload=trace". The logging statements that
>>>> I'm again having trouble reconciling the conversion of non-product
>>>> logging into product logging. Really not sure it is desirable or useful.
>>>>
>>>> More generally although most of our "tracing" flags start with Trace
>>>> (some start with Print), that should be read simply as "log" or
>>>> "report" in my opinion. So in the sense that the UL framework defines:
>>>> info, debug, trace; it is certainly not the case that our "trace"
>>>> flags should automatically map to a "trace" log level in the first
>>>> instance. For something like classloading/unloading I would still
>>>> expect to see basic information at the info level, more detailed info
>>>> at the debug level, and so forth. So why are we not doing the basic
>>>> class-unloading logging at the info level?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>> could have been previously been triggered in Product mode are all now
>>>>> accessed by "-Xlog:classunload=debug". (We are updating logging, but we
>>>>> are not updating the functionality of the old logging options. Old
>>>>> options such as TraceClassUnloading are being aliased so they may
>>>>> continue to work in *exactly* the way they did before.)
>>>>>
>>>>> Thanks,
>>>>> Max
>>>>>