RFR: 8149996: TraceLoaderConstraints has been converted to Unified Logging.

[David Holmes]

Follow ups below ...

On 17/03/2016 1:00 AM, Max Ockner wrote:
> Comments below.
>
> On 3/16/2016 2:11 AM, David Holmes wrote:
>> Hi Max,
>>
>> tl;dr: you can push this as-is (modulo fixing copyright years) but I
>> have some further queries/discussion points below. :)
>>
>> Meta question: should we consider using "classload, constraints"
>> instead of "loaderconstraints" ?
>>
> I think this would be appropriate.

Ok.

>> On 16/03/2016 1:00 AM, Max Ockner wrote:
>>> Hello again,
>>>
>>> new webrev: http://cr.openjdk.java.net/~mockner/8149996.05/
>>>   - added resource marks back in
>>>   - reverted to using outputStreams instead of log_info macro
>>>
>>> I ran into an issue with the outputStream eating its output when I
>>> removed " ]\n" from the end of some of the messages, solved by changing
>>> "out->print" to "out->print_cr".
>>
>> Aside: it is actually more efficient to have the \n in the string than
>> to use the print_cr form. :)
>>
> Thanks for pointing this out. I can change it.

It was just an observation - we don't pay this much attention elsewhere.

>> Nit: is there a reason to remove the initial capitals from the log
>> messages? I find the capitals are a visual aid when trying to find the
>> end of the decorators.
>>
>> Aside 2: I notice we have now removed the only example of a product_rw
>> flag. I wonder if we should rip out product_rw support? (Separate RFE
>> of course). I'm also wondering if our logging flags are
>> visible/modifiable via the management tools like manageable/product_rw
>> flags are?
>>
>  From an email discussion with Marcus:
> "There is a DCMD to reconfigure logging during runtime. It's all baked
> into a single command called VM.log and it works similar to the -Xlog
> command, but all of the parameters are named, so -Xlog:gc:gclog.txt
> would be VM.log output=gclog.txt what=gc."

Thanks.

>> With regard to the test I'm not sure those "loaders" are going to
>> continue to be valid once Jake has integrated. So you might want to
>> try this on a Jake build before finalising it.
>>
>
> Do you think this is important to test before submitting?

It might stop working next week so yes I think it worth checking this now.

Thanks,
David

>> Thanks,
>> David
>>
>>> I thought about writing a log_info_rm which evaluates to a code block
>>> instead of a function, but I think one of the benefits of the current
>>> macros is that they allow two variable argument lists in the same
>>> expression. Returning a write function is important for handling the
>>> format string and its variable length argument list of format
>>> %substitutions.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>> On 3/9/2016 11:29 PM, David Holmes wrote:
>>>> On 10/03/2016 8:03 AM, Coleen Phillimore wrote:
>>>>>
>>>>> http://cr.openjdk.java.net/~mockner/8149996.02/src/share/vm/classfile/loaderConstraints.cpp.frames.html
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Why did you take out the ResourceMarks at line 130, 160, 231 and 244,
>>>>> 297, 308 and 357?
>>>>
>>>> Yes these should all be of the form:
>>>>
>>>> if (log_is_enabled(...)) {
>>>>   ResourceMark rm;
>>>>   // log stuff
>>>> }
>>>>
>>>>> Unfortunately, we really need log_info_rm(tag)("String") call because
>>>>> all those places need a ResourceMark.
>>>>>
>>>>> Anytime we call name()->as_C_string() there needs to be a ResourceMark
>>>>> in the scope of the call.
>>>>
>>>> Which means that a log_info_rm() call wouldn't help because we'd still
>>>> be calling as_C_string in the caller. :(
>>>>
>>>> David
>>>>
>>>>>
>>>>> Coleen
>>>>>
>>>>> On 3/9/16 3:47 PM, Max Ockner wrote:
>>>>>> Hello again,
>>>>>>
>>>>>> Please review this change. TraceLoaderConstraints has been converted
>>>>>> to Unified Logging and can be accessed using
>>>>>> '-Xlog:loaderconstraints=info'. TraceLoaderConstraints has been added
>>>>>> to the logging alias table.
>>>>>>
>>>>>> There are 0 lines of output for java -version, and ~10 lines from
>>>>>> LoaderConstraintsTest.java
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8149996
>>>>>> webrev: http://cr.openjdk.java.net/~mockner/8149996.02/
>>>>>>
>>>>>> Testing: jtreg runtime. Added new test which triggers logging for
>>>>>> loaderconstraints by forcing class unloading. No other references to
>>>>>> TraceLoaderConstraints found in hotspot/test, jdk/test , or in the
>>>>>> noncolo tests.
>>>>>>
>>>>>> Thanks,
>>>>>> Max
>>>>>
>>>
>