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

[David Holmes]

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

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

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

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
>