RFR(S): 8160064: StackWalker implementation added logging option without using UL

[David Holmes]

On 28/09/2016 9:01 AM, Coleen Phillimore wrote:
>
>
> On 9/27/16 6:54 PM, David Holmes wrote:
>> Hi Rachel,
>>
>> On 28/09/2016 5:47 AM, Rachel Protacio wrote:
>>> Hi,
>>>
>>> Please review this small fix correcting the StackWalker print output
>>> mechanism by moving it to Unified Logging. Includes a new jtreg test,
>>> and passes JPRT.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8160064
>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8160064/
>>
>> Was there a specific reason to make this available in product rather
>> than just "develop" as the existing flag was?
>>
>>       if (!ShowHiddenFrames && (skip_hidden_frames(mode) ||
>> get_caller_class(mode))) {
>>         if (method->is_hidden()) {
>> !         {
>> !           ResourceMark rm;
>> !           outputStream* st = Log(stackwalk)::trace_stream();
>>
>> This should check if logging is enabled before potentially wasting
>> time on the ResourceMark etc. Ditto for other places. Did you revert
>> this due to Coleen's query?
>
> No, my query was that I was suggesting adding the if
> (log_is_enabled(Trace, stackwalk)) to the code.

Ah sorry, the way you included the fragment and phrased the query I 
thought you were asking if the check should be removed.

>> We want to minimise what happens when logging is not enabled - ideally
>> it would be 100% free.
>
> yes.
>>
>> I also query why all the logging entries are at the trace level?
>
> It seems like this could generate quite a bit of output => trace.

I continue to have issues with this. I really believe that for any given 
log level there should be info, debug, and trace depending on the level 
of detail for that particular logging flag. So for any given flag info 
should always produce some output. Otherwise you have no way to know 
what level you need to set to get any output.

Cheers,
David


> Coleen
>
>> Thanks,
>> David
>>
>>> Thank you!
>>> Rachel
>