lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "George Aroush" <geo...@aroush.net>
Subject RE: Problems with patch for LUCENENET-106
Date Tue, 06 Jan 2009 00:46:43 GMT
The fix should be in the implementation / port of WeakHashMap, and not in
Lucene.Net base code.

I believe Luc is working on providing an alternative patch / fix.

-- George

> -----Original Message-----
> From: Timothy Januario [mailto:Timothy.Januario@BDMetrics.com] 
> Sent: Monday, January 05, 2009 10:19 AM
> To: lucene-net-dev@incubator.apache.org
> Subject: RE: Problems with patch for LUCENENET-106
> 
> I stand corrected.  I do see the possibility of the collision 
> as a potentially serious bug.  The java version still uses 
> the object as the key which means that if the hashcode is 
> identical across all instances, it just becomes less 
> efficient.  In the current .NET implementation, it stays very 
> efficient, it would just only ever have a single instance in 
> the cache which is a very different and incorrect result.
> 
> -----Original Message-----
> From: Eyal Post [mailto:eyalpost@epocalipse.com]
> Sent: Saturday, January 03, 2009 1:56 PM
> To: lucene-net-dev@incubator.apache.org
> Subject: RE: Problems with patch for LUCENENET-106
> 
> I tend to agree with Luc. 
> Hashtables don't rely on small chance for collisions to 
> operate correctly.
> They only rely on it to operate efficiently. 
> This means you can store 2 elements with the same hashcode in 
> a hashtable and they won't override each other, but the 
> hashtable lookups will be less efficient with collisions.
> The problem with code provided in the patch is that it uses 
> the hashcode as the key in the hashtable and altough the 
> chance of collisions is small once they happen the results 
> are undefined.
> I think the best way to solve that is to use WeakEntry as the 
> key and override both GetHashCode and Equals. GetHashCode 
> should return the HashCode field and Equals should compare the Keys
> 
> 
> Eyal Post
>  
> 
> > -----Original Message-----
> > From: George Aroush [mailto:george@aroush.net]
> > Sent: Friday, January 02, 2009 0:52 AM
> > To: lucene-net-dev@incubator.apache.org
> > Subject: RE: Problems with patch for LUCENENET-106
> > 
> > Hi Luc,
> > 
> > I have to agree with Tim.
> > 
> > The .NET's implementation / port of WeakHashMap in Lucene.Net is 
> > consistent with Java Lucene.  If there is a "very" small 
> chance of a 
> > hash key collusion, it is also possible in the Java version 
> of Lucene.  
> > Here is why.
> > 
> > In Java, when WeakHashMap.put(Object key, Object value) is called, 
> > this function calls key.hashCode() to get a hash value (I 
> had a look 
> > at Java's source code for WeakHashMap.) The same logic is 
> happening in 
> > .NET's implementation / port of WeakHashMap.  In addition, at least 
> > for now, the "key" is always of type IndexReader.  Looking in 
> > IndexReader code, there is no override of Java::hashCode() / 
> > C#::GetHashCode().
> >  So, unless if Java's
> > hashCode() guaranties uniqueness (which it doesn't), then the 
> > possibility of a hash value collusion also exists in the 
> Java version 
> > of Lucene.  If this is the case, then we have a valid 
> defect in Lucene 
> > and it should be addressed.
> > 
> > Erik, do you agree, can you comment?
> > 
> > Regarding #2, & #3, I agree that the .NET implementation of 
> > WeakHashMap could have been confined to just get() / put().
> > Since you already have a patch for Lucene.Net 2.1, please 
> submit a new 
> > JIRA issue and attach your patch to it (when you are 
> ready.)  I look 
> > forward to your patch.
> > 
> > Regards, and Happy New Year everyone!
> > 
> > -- George
> > 
> > 
> > > -----Original Message-----
> > > From: Timothy Januario [mailto:Timothy.Januario@BDMetrics.com]
> > > Sent: Wednesday, December 31, 2008 9:44 AM
> > > To: lucene-net-dev@incubator.apache.org
> > > Subject: RE: Problems with patch for LUCENENET-106
> > > 
> > > Luc,
> > > I'm not sure that I completely agree with your assessment of the 
> > > WeakHashMap.
> > > 
> > > It does assume that the hash value is unique which is a
> > correct usage.  
> > > It is the responsibility of
> > > object.GetHashCode() to return a unique value.  This is a
> > problem in
> > > any Hashtable implementation and although I haven't seen
> > the internal
> > > java code, I assume that it has the same limitation.  The
> > object would
> > > be the key only superficially as the hashcode would be used
> > internally
> > > as well (based on the name of the object, WeakHashMap, anyway).
> > > If this is incorrect, I apologize for the noise.  I did, however, 
> > > notice that GetHashCode() is not overridden in the 
> IndexReader and 
> > > since the .NET framework does not guarantee a unique value
> > (according
> > > to the documentation for oject.GetHashCode()), this could
> > present the
> > > problem that you have identified.  Is there a need to guarantee 
> > > uniqueness by overriding the method or does anyone know if
> > the value
> > > is always unique already?  When the Cache object was
> > implemented with
> > > Hashtable prior to this patch, the same problem would have been 
> > > inherent with that implementation (of course with the
> > additional bonus
> > > of the memory leak ;)) and I don't think I ever saw collisions 
> > > although quite honestly I was never really looking for them.
> > > 
> > > Also, you said that the call to WeakHashMap.Keys could 
> return NULL 
> > > values.  This may be the reason why the Java implementation
> > supports
> > > null keys.  It does seem to be a possibility unless the
> > call to Keys
> > > checks that WeakReference.Key.Target is not null at the time of 
> > > iteration (this would eliminate this possibility since we 
> would now 
> > > have a strong reference to the object ie: object key = 
> > > entries[i].Key.Target; if (object != null) 
> keys.Add(object);).  The 
> > > same problem would exist with the Values property.
> > > 
> > > Also, optimizing for few IndexReaders in the WeakHashMap is an 
> > > assumption that specializes the class for IndexReaders.
> > > There is no guarantee that this will be the only use case in the 
> > > future.  The Java documentation says that the same initial
> > parameters
> > > (loadfactor and capacity) as HashMap are used which
> > suggests that it
> > > should be left in the .NET implementation with the same
> > parameters as
> > > the default Hashtable which is its underlying container.
> > > 
> > > Comments?
> > > -tim
> > > 
> > > -----Original Message-----
> > > From: Vanlerberghe, Luc [mailto:Luc.Vanlerberghe@bvdep.com]
> > > Sent: Wednesday, December 31, 2008 4:48 AM
> > > To: lucene-net-dev@incubator.apache.org
> > > Subject: Problems with patch for LUCENENET-106
> > > 
> > > Hi,
> > > 
> > > I reviewed the fix for LUCENENET-106 and I see a couple of issues:
> > > 
> > > 1. The implementation of WeakHashMap in SupportClass.cs has
> > a bug in
> > > that it assumes all keys will have a unique HashCode.
> > > Although the chances of this occurring are *very* small, it
> > cannot be
> > > guaranteed. A collision would mean that the cached value for one 
> > > indexReader could be returned as cached value for another
> > indexReader
> > > (using the way this class is used in Lucene as example)
> > > 
> > > 2. This class attempts to be a complete port of the 
> > > java.util.WeakHashMap, but:
> > > - A correct implementation is difficult to implement, and 
> .Net does 
> > > not have an equivalent of java.lang.ref.ReferenceQueue to
> > accelerate
> > > the cleanup process.
> > > - Since the behaviour of the WeakHashMap is closely tied to the 
> > > behaviour of the garbage collector, it is very difficult to test 
> > > properly, if at all.
> > > - Lucene only uses the get(Object key) and put(K key, V
> > value) methods
> > > - In Lucene, the keys are IndexReader instances.  In normal
> > use cases
> > > there will be only 1 IndexReader live, and perhaps another one if 
> > > 'warming up' is used before switching IndexSearchers 
> after an index 
> > > update.
> > > - If this class is presented as a complete port of 
> > > java.util.WeakHashMap, users might assume this is
> > production quality
> > > code and copy/paste its code in their own projects.
> > > Any further bugs found might harm the credibility of the Lucene 
> > > project, even if those sections are never used in Lucene.
> > (e.g.: The
> > > collection returned by Keys might contain null values.  
> There's no 
> > > guarantee the GC won't intervene between the call to
> > Cleanup and the
> > > calls to keys.Add(w.Key.Target).
> > > 
> > > 3. Most support classes that are needed for the conversion
> > of java to
> > > .Net are in the anonymous namespace, but this one is in 
> > > Lucene.Net.Util.  I would propose to keep the namespaces
> > corresponding
> > > to the original java packages clean and either put all
> > support classes
> > > in the anonymous namespace or in a Lucene.Net specific one 
> > > (Lucene.Net.DotNetPort ?) (I see that George moved it already to 
> > > SupportClass, thanks George!)
> > > 
> > > I have been using the java version of Lucene in a project
> > for a long
> > > time.
> > > For a new project that is currently under development, we will be 
> > > using the .Net version.
> > > For now we are using the 2.1 version using a custom patch
> > to avoid the
> > > memory leak problem.
> > > 
> > > I'll post an up to date version of this patch as a
> > replacement for the
> > > current implementation of Lucene.Net.Util.WeakHashMap 
> sometime next 
> > > week...
> > > - It doesn't try to be a complete implementation of
> > > WeakHashMap: only the methods strictly necessary are
> > implemented, all
> > > other methods throw a NotImplementedException. (It should
> > probably be
> > > renamed to SimplifiedWeakHashMap or something)
> > > - It's optimized for the 'low number of keys' case.
> > > - It's simple code, but I want to test it thoroughly first.  
> > > The original patch was put directly in the FieldCacheImpl
> > code, I want
> > > to make sure I didn't make some stupid mistake while 
> converting it.
> > > 
> > > I have an account on Jira, but I don't have rights to reopen the 
> > > issue...
> > > 
> > > Regards,
> > > 
> > > Luc
> > > 
> > > 
> > > -----Original Message-----
> > > From: Digy (JIRA) [mailto:jira@apache.org]
> > > Sent: zaterdag 27 december 2008 19:47
> > > To: lucene-net-dev@incubator.apache.org
> > > Subject: [jira] Closed: () Lucene.NET (Revision: 603121) 
> is leaking 
> > > memory
> > > 
> > > 
> > >      [
> > > https://issues.apache.org/jira/browse/LUCENENET-106?page=com.a
> > tlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
> > > 
> > > Digy closed LUCENENET-106.
> > > --------------------------
> > > 
> > >     Resolution: Fixed
> > >       Assignee: Digy
> > > 
> > > Patches are applied.
> > > 
> > > DIGY.
> > > 
> > > > Lucene.NET (Revision: 603121) is leaking memory
> > > > -----------------------------------------------
> > > >
> > > >                 Key: LUCENENET-106
> > > >                 URL: 
> > > https://issues.apache.org/jira/browse/LUCENENET-106
> > > >             Project: Lucene.Net
> > > >          Issue Type: Bug
> > > >         Environment: .NET 2.0
> > > >            Reporter: Anton K.
> > > >            Assignee: Digy
> > > >            Priority: Critical
> > > >         Attachments: DIGY-FieldCacheImpl.patch, Digy.rar,
> > > luceneSrc_memUsage.patch, Paches for v2.3.1.rar,
> > > WeakHashTable+FieldCacheImpl.rar, WeakReferences.rar
> > > >
> > > >
> > > > readerCache Hashtable field (see FieldCacheImpl.cs) never
> > > releases some hash items that have closed IndexReader
> > object as a key. 
> > > So a lot of Term instances are never released.
> > > > Java version of Lucene uses WeakHashMap and therefore
> > > doesn't have this problem.
> > > > This bug can be reproduced only when Sort functionality
> > > used during search. 
> > > > See following link for additional information.
> > > > http://www.gossamer-threads.com/lists/lucene/java-user/55681
> > > > Steps to reproduce:
> > > > 1)Create index
> > > > 2) Modify index by IndexWiter; Close IndexWriter
> > > > 3) Use IndexSearcher for searching with Sort; Close InexSearcher
> > > > 4) Go to step 2
> > > > You'll get OutOfMemoryException after some time of running
> > > this algorithm.
> > > 
> > > --
> > > This message is automatically generated by JIRA.
> > > -
> > > You can reply to this email to add a comment to the issue online.
> > > 
> > > 
> > > 
> > 
> > 
> 


Mime
View raw message