lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael McCandless <luc...@mikemccandless.com>
Subject Re: Unnecessary messages creation by LogMergePolicy
Date Fri, 05 Dec 2008 16:45:41 GMT

I like the method, but how about the name verbose(), ie:

   if (verbose())
     ...

Mike

Shai Erera wrote:

> I'll open an issue and work out a patch.
>
> Basically it means infoStream != null, although in LogMergePolicy I  
> might add a specific method for that, because the messages are  
> output if the IndexWriter member is not null and its infoStream is  
> not null (this check is done by IndexWriter).
>
> Therefore I think I'll add a method to IndexWriter messagesEnabled()  
> which returns true if the infoStream is not null for use by other  
> classes (rather than the implicit iw.getInfoStream() != null). BTW,  
> getInfoStream() is not called by any class in Lucene, except one  
> test class.
>
> What do you think about adding this method, and its name?
>
> On Fri, Dec 5, 2008 at 3:35 PM, Michael McCandless <lucene@mikemccandless.com 
> > wrote:
>
> I agree, it is a best practice and we should follow it.  Can you  
> work out a patch & open an issue?  I assume this means "if  
> (infoStream != null)..." in this case.
>
> Mike
>
>
> Shai Erera wrote:
>
> Hi
>
> As I looked at the code in LogMergePolicy (and its sub-classes), I  
> came across such lines:
>
>    message("findMergesToExpungeDeletes: " + numSegments + "  
> segments");
>
> Those lines print to the info stream (eventually) if it's not null.
>
> If one follows Java logging best practices, then any logging message  
> should look similar to this:
> if (logger.isLoggable(Level)) {
>   logger.log(Level, msg);
> }
>
> The reason is that when logging messages, one usually does not pay  
> attention to any performance issues, like String concatenation.  
> Therefore, checking if logging is enabled saves building the String  
> just to discover later that logging is not enabled.
>
> I haven't checked other places in the code, because I'd like to get  
> the committers opinion on this first. Imo, those strings are created  
> unnecessarily (the above message creates 5 strings) since most of  
> the time the info stream is null.
>
> I can provide a patch to fix it by first checking if logging (or  
> whatever other name you'd like to give it) is enabled before  
> attempting to output any message. The LogMergePolicy classes are one  
> example that I've run at, but I'm sure there are other places in the  
> code.
>
> I don't foresee any great performance improvements by this fix,  
> except just following best practices.
>
> What do you think?
>
> Shai
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org


Mime
View raw message