RFR: JDK-8143255: Remove debug logging from SymbolTable::unlink() and SymbolTable::possibly_parallel_unlink()

Thu Nov 19 15:23:10 UTC 2015

On 2015-11-19 16:00, Coleen Phillimore wrote:
>
> Thanks Bengt for the reminder. I did add this code when moving Symbols 
> out of PermGen.  lol  The GC people did not want to see this output so 
> I added Verbose and WizardMode for extra good measure.  I have never 
> looked at this output again.  I think it can be removed. There are 
> similar statistics in PrintSymbolTableSizeHistogram that are more 
> useful and less noisy.
>
> Webrev .01 looks good.

Thanks, Coleen! I'll go ahead and push this now.

Bengt

>
> For UL conversion, yes, absolutely, we are looking at removing some 
> "logging" that is actually left-over debugging that may or may not 
> help a new person debugging the code.   In this case, it's better not 
> to have this clutter.  It's costly to maintain and have to look at in 
> such a large (getting larger) codebase.
>
> Thanks,
> Coleen
>
> On 11/19/15 3:13 AM, Bengt Rutisson wrote:
>>
>>
>> On 2015-11-19 08:54, David Holmes wrote:
>>> On 19/11/2015 5:41 PM, Bengt Rutisson wrote:
>>>>
>>>> Hi David,
>>>>
>>>> On 2015-11-18 22:42, David Holmes wrote:
>>>>> Hi Bengt,
>>>>>
>>>>> On 19/11/2015 5:47 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi everyone,
>>>>>>
>>>>>> Could I have a couple of reviews for this patch to remove some 
>>>>>> debugging
>>>>>> code?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~brutisso/8143255/webrev.00/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8143255
>>>>>>
>>>>>>  From the bug report:
>>>>>>
>>>>>> The logging inside SymbolTable::unlink() and
>>>>>> SymbolTable::possibly_parallel_unlink() is guarded by Verbose and
>>>>>> WizardMode, which are both develop flags. This means that the log 
>>>>>> output
>>>>>> is not available in product builds. So, this logging is most 
>>>>>> likely just
>>>>>> leftover debugging code.
>>>>>
>>>>> As this is primarily guarded by PrintGCDetails the GC team owns this
>>>>> as far as I am concerned and so they can do as they desire. 
>>>>> However ...
>>>>
>>>> This is runtime code, so I think it was a mistake to guard this 
>>>> logging
>>>> with PrintGCDetails. But since I am actively working on replacing 
>>>> the GC
>>>> logging with the new unified logging framework I stumbled across this
>>>> piece of code.
>>>
>>> Well someone obviously considered this of interest when doing GC 
>>> logging :)
>>
>> It was added by the runtime team in this change:
>>
>> "Use native memory and reference counting to implement SymbolTable"
>> https://bugs.openjdk.java.net/browse/JDK-6990754
>>
>> See:
>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/3582bf76420e#l68.89
>>
>> And then changed by this change:
>>
>> "Symbol printouts shouldnt be under PrintGCDetails"
>> https://bugs.openjdk.java.net/browse/JDK-7024584
>>
>> See:
>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/df1347358fe6
>>
>> So, I would say no. This was never of interest for GC logging.
>>
>>
>>>
>>>> I discussed it with the runtime team and they did not consider the
>>>> logging useful. So, rather than trying to figure out a conversion 
>>>> to the
>>>> new logging framework it seemed better to remove it.
>>>>
>>>>>
>>>>> Most, if not all, non-product logging code could be characterized as
>>>>> "leftover debugging code" - but that doesn't mean it should all be
>>>>> ripped out. The use of Verbose && WizardMode does seem a little
>>>>> excessive in this case - one should suffice. As documented the aim is
>>>>> to ensure that this logging, while logically wanted as part of
>>>>> PrintGCDetails, is not included in the product mode because people
>>>>> parse that logging information.
>>>>>
>>>>>> In the GC team we don't use this logging (even though it is for some
>>>>>> reason guarded by PrintGCDetails too) and I discussed it with 
>>>>>> Coleen and
>>>>>> Rachel in the Runtime team. The Runtime team also don't use the 
>>>>>> logging.
>>>>>
>>>>> This kind of logging is used to expose additional details while
>>>>> testing/debugging issues. It isn't expected to be used on a regular
>>>>> basis, and may well have not been used for years, or may have last
>>>>> been used extensively by people no longer working on OpenJDK.
>>>>
>>>> Agreed. And unused code is just noise in the code base, so it is 
>>>> nice to
>>>> get it cleaned up.
>>>
>>> But on that basis the bulk of non-product logging code would be a 
>>> candidate for deletion.
>>
>> I think useful logging should be left (and preferably made available 
>> in product mode) and useless logging should be removed.
>>
>>>
>>>>>
>>>>>> It is pretty straight forward to add this logging back if it is ever
>>>>>> needed for a debugging session. Until then we should remove this 
>>>>>> from
>>>>>> the code base.
>>>>>
>>>>> Mostly likely someone would not even know it ever existed and would
>>>>> simply re-write a new piece of logging code - which might even get
>>>>> checked in. And the cycle repeats. ;-)
>>>>
>>>> That's my point. Not that the cycle repeats, but that it is very 
>>>> easy to
>>>> implement this code again if it would ever be needed. The benefit of
>>>> having it checked in is very little compared to the negative effect on
>>>> the reading speed for all developers that have to read this code.
>>>
>>> Something to consider on a case by case basis I hope. Else, as I 
>>> said, a lot of non-product logging should go.
>>
>> Definitely a case by case thing!
>>
>> We just have to remember that there is a significant cost to having 
>> useless code just laying around.
>>
>> Cheers,
>> Bengt
>>
>>>
>>> Cheers,
>>> David
>>>
>>>
>>>>>
>>>>> Given the trend elsewhere I'm a little surprised this isn't simply
>>>>> being included in the product mode logging. :)
>>>>
>>>> In my opinion what we are trying do do elsewhere is to say that
>>>> non-product logging should be used only for performance reasons or 
>>>> when
>>>> we worry about the memory footprint. Most of the time we want the
>>>> logging to be available in product builds if it in any way produces
>>>> useful information.
>>>>
>>>> In this case it is not producing useful information in product or
>>>> non-product builds, so there is no reason to move it to product level
>>>> logging.
>>>>
>>>> Cheers,
>>>> Bengt
>>>>
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>
>>
>