You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cassandra.apache.org by Radim Kolar <hs...@filez.com> on 2012/11/22 18:51:09 UTC
slf4j
instead of this:
if (logger.isDebugEnabled())
logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
(System.currentTimeMillis() - start) + " ms.");
do this:
logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
(System.currentTimeMillis() - start));
easier to read.
Re: slf4j
Posted by Eric Charles <er...@apache.org>.
More guidelines from the src:
http://www.slf4j.org/faq.html#logging_performance
I was thrilled reading "...the second form will outperform the first
form by a factor of at least 30..."
On 23/11/2012 15:18, Jonathan Ellis wrote:
> I prefer the concise approach when no evaluation needs to be performed
> on the method arguments, but when it does I prefer the explicit
> isDebugEnabled check, or else reviewers need to think each time they
> see one, "is this a hot code path where we can afford to be sloppy, or
> not?"
>
> On Fri, Nov 23, 2012 at 9:14 AM, Stephen Connolly
> <st...@gmail.com> wrote:
>> On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
>>
>>> instead of this:
>>>
>>> if (logger.isDebugEnabled())
>>> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
>>> (System.currentTimeMillis() - start) + " ms.");
>>>
>>> do this:
>>> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
>>> (System.currentTimeMillis() - start));
>>>
>>
>> Yes, but until/unless the JVM elides the function call because that logger
>> is not enabled fro debug, you will incur the penalty of new
>> Object[]{string,new Long(System.currentTimeMillis() - start).
>>
>> On top of that, when debug is enabled you incur the cost of formatting the
>> string, including the {} search & replace.
>>
>> On a long running production system, you are correct that it should
>> *eventually* be equivalent, and I am not saying one way or the other
>> whether this should/shouldn't be changed... more just pointing out that
>> there are consequences to what might appear to be a trivial change... I'll
>> let the c* devs chime in as to what their strong opinion is in this regard
>> as they should have more experience handling high loads and I would love to
>> know which pattern I should be using in my code (FWIW I tend to favour your
>> approach to the if (debugEnabled) guard... but I always wonder ;-) )
>>
>> -Stephen
>>
>>
>>>
>>> easier to read.
>>>
>
>
>
Re: slf4j
Posted by Stephen Connolly <st...@gmail.com>.
On 23 November 2012 17:17, Dave Brosius <db...@mebigfatguy.com> wrote:
> There are actually 2 arguments the OP is making.. the second, using
>
> {} : {}
>
>
> over
>
>
> descriptor + ": " + (System.currentTimeMillis() - start)
>
> is reasonable.
>
>
Having had to do a search & replace over an entire code base (thank you
IntelliJ structural search) to remove the string concat in logging due to
the GC pressure it caused. I tend to agree on that second point.
>
> On 11/23/2012 10:18 AM, Jonathan Ellis wrote:
>
>> I prefer the concise approach when no evaluation needs to be performed
>> on the method arguments, but when it does I prefer the explicit
>> isDebugEnabled check, or else reviewers need to think each time they
>> see one, "is this a hot code path where we can afford to be sloppy, or
>> not?"
>>
>> On Fri, Nov 23, 2012 at 9:14 AM, Stephen Connolly
>> <stephen.alan.connolly@gmail.**com <st...@gmail.com>>
>> wrote:
>>
>>> On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
>>>
>>> instead of this:
>>>>
>>>> if (logger.isDebugEnabled())
>>>> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
>>>> (System.currentTimeMillis() - start) + " ms.");
>>>>
>>>> do this:
>>>> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
>>>> (System.currentTimeMillis() - start));
>>>>
>>>> Yes, but until/unless the JVM elides the function call because that
>>> logger
>>> is not enabled fro debug, you will incur the penalty of new
>>> Object[]{string,new Long(System.currentTimeMillis(**) - start).
>>>
>>> On top of that, when debug is enabled you incur the cost of formatting
>>> the
>>> string, including the {} search & replace.
>>>
>>> On a long running production system, you are correct that it should
>>> *eventually* be equivalent, and I am not saying one way or the other
>>> whether this should/shouldn't be changed... more just pointing out that
>>> there are consequences to what might appear to be a trivial change...
>>> I'll
>>> let the c* devs chime in as to what their strong opinion is in this
>>> regard
>>> as they should have more experience handling high loads and I would love
>>> to
>>> know which pattern I should be using in my code (FWIW I tend to favour
>>> your
>>> approach to the if (debugEnabled) guard... but I always wonder ;-) )
>>>
>>> -Stephen
>>>
>>>
>>> easier to read.
>>>>
>>>>
>>
>>
>
Re: slf4j
Posted by Jonathan Ellis <jb...@gmail.com>.
Agreed.
On Fri, Nov 23, 2012 at 11:17 AM, Dave Brosius <db...@mebigfatguy.com> wrote:
> There are actually 2 arguments the OP is making.. the second, using
>
> {} : {}
>
>
> over
>
>
> descriptor + ": " + (System.currentTimeMillis() - start)
>
> is reasonable.
>
>
>
> On 11/23/2012 10:18 AM, Jonathan Ellis wrote:
>>
>> I prefer the concise approach when no evaluation needs to be performed
>> on the method arguments, but when it does I prefer the explicit
>> isDebugEnabled check, or else reviewers need to think each time they
>> see one, "is this a hot code path where we can afford to be sloppy, or
>> not?"
>>
>> On Fri, Nov 23, 2012 at 9:14 AM, Stephen Connolly
>> <st...@gmail.com> wrote:
>>>
>>> On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
>>>
>>>> instead of this:
>>>>
>>>> if (logger.isDebugEnabled())
>>>> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
>>>> (System.currentTimeMillis() - start) + " ms.");
>>>>
>>>> do this:
>>>> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
>>>> (System.currentTimeMillis() - start));
>>>>
>>> Yes, but until/unless the JVM elides the function call because that
>>> logger
>>> is not enabled fro debug, you will incur the penalty of new
>>> Object[]{string,new Long(System.currentTimeMillis() - start).
>>>
>>> On top of that, when debug is enabled you incur the cost of formatting
>>> the
>>> string, including the {} search & replace.
>>>
>>> On a long running production system, you are correct that it should
>>> *eventually* be equivalent, and I am not saying one way or the other
>>> whether this should/shouldn't be changed... more just pointing out that
>>> there are consequences to what might appear to be a trivial change...
>>> I'll
>>> let the c* devs chime in as to what their strong opinion is in this
>>> regard
>>> as they should have more experience handling high loads and I would love
>>> to
>>> know which pattern I should be using in my code (FWIW I tend to favour
>>> your
>>> approach to the if (debugEnabled) guard... but I always wonder ;-) )
>>>
>>> -Stephen
>>>
>>>
>>>> easier to read.
>>>>
>>
>>
>
--
Jonathan Ellis
Project Chair, Apache Cassandra
co-founder of DataStax, the source for professional Cassandra support
http://www.datastax.com
Re: slf4j
Posted by Dave Brosius <db...@mebigfatguy.com>.
There are actually 2 arguments the OP is making.. the second, using
{} : {}
over
descriptor + ": " + (System.currentTimeMillis() - start)
is reasonable.
On 11/23/2012 10:18 AM, Jonathan Ellis wrote:
> I prefer the concise approach when no evaluation needs to be performed
> on the method arguments, but when it does I prefer the explicit
> isDebugEnabled check, or else reviewers need to think each time they
> see one, "is this a hot code path where we can afford to be sloppy, or
> not?"
>
> On Fri, Nov 23, 2012 at 9:14 AM, Stephen Connolly
> <st...@gmail.com> wrote:
>> On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
>>
>>> instead of this:
>>>
>>> if (logger.isDebugEnabled())
>>> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
>>> (System.currentTimeMillis() - start) + " ms.");
>>>
>>> do this:
>>> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
>>> (System.currentTimeMillis() - start));
>>>
>> Yes, but until/unless the JVM elides the function call because that logger
>> is not enabled fro debug, you will incur the penalty of new
>> Object[]{string,new Long(System.currentTimeMillis() - start).
>>
>> On top of that, when debug is enabled you incur the cost of formatting the
>> string, including the {} search & replace.
>>
>> On a long running production system, you are correct that it should
>> *eventually* be equivalent, and I am not saying one way or the other
>> whether this should/shouldn't be changed... more just pointing out that
>> there are consequences to what might appear to be a trivial change... I'll
>> let the c* devs chime in as to what their strong opinion is in this regard
>> as they should have more experience handling high loads and I would love to
>> know which pattern I should be using in my code (FWIW I tend to favour your
>> approach to the if (debugEnabled) guard... but I always wonder ;-) )
>>
>> -Stephen
>>
>>
>>> easier to read.
>>>
>
>
Re: slf4j
Posted by Jonathan Ellis <jb...@gmail.com>.
I prefer the concise approach when no evaluation needs to be performed
on the method arguments, but when it does I prefer the explicit
isDebugEnabled check, or else reviewers need to think each time they
see one, "is this a hot code path where we can afford to be sloppy, or
not?"
On Fri, Nov 23, 2012 at 9:14 AM, Stephen Connolly
<st...@gmail.com> wrote:
> On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
>
>> instead of this:
>>
>> if (logger.isDebugEnabled())
>> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
>> (System.currentTimeMillis() - start) + " ms.");
>>
>> do this:
>> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
>> (System.currentTimeMillis() - start));
>>
>
> Yes, but until/unless the JVM elides the function call because that logger
> is not enabled fro debug, you will incur the penalty of new
> Object[]{string,new Long(System.currentTimeMillis() - start).
>
> On top of that, when debug is enabled you incur the cost of formatting the
> string, including the {} search & replace.
>
> On a long running production system, you are correct that it should
> *eventually* be equivalent, and I am not saying one way or the other
> whether this should/shouldn't be changed... more just pointing out that
> there are consequences to what might appear to be a trivial change... I'll
> let the c* devs chime in as to what their strong opinion is in this regard
> as they should have more experience handling high loads and I would love to
> know which pattern I should be using in my code (FWIW I tend to favour your
> approach to the if (debugEnabled) guard... but I always wonder ;-) )
>
> -Stephen
>
>
>>
>> easier to read.
>>
--
Jonathan Ellis
Project Chair, Apache Cassandra
co-founder of DataStax, the source for professional Cassandra support
http://www.datastax.com
Re: slf4j
Posted by Stephen Connolly <st...@gmail.com>.
On 22 November 2012 17:51, Radim Kolar <hs...@filez.com> wrote:
> instead of this:
>
> if (logger.isDebugEnabled())
> logger.debug("INDEX LOAD TIME for " + descriptor + ": " +
> (System.currentTimeMillis() - start) + " ms.");
>
> do this:
> logger.debug("INDEX LOAD TIME for {} : {} ms.", descriptor,
> (System.currentTimeMillis() - start));
>
Yes, but until/unless the JVM elides the function call because that logger
is not enabled fro debug, you will incur the penalty of new
Object[]{string,new Long(System.currentTimeMillis() - start).
On top of that, when debug is enabled you incur the cost of formatting the
string, including the {} search & replace.
On a long running production system, you are correct that it should
*eventually* be equivalent, and I am not saying one way or the other
whether this should/shouldn't be changed... more just pointing out that
there are consequences to what might appear to be a trivial change... I'll
let the c* devs chime in as to what their strong opinion is in this regard
as they should have more experience handling high loads and I would love to
know which pattern I should be using in my code (FWIW I tend to favour your
approach to the if (debugEnabled) guard... but I always wonder ;-) )
-Stephen
>
> easier to read.
>