lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shad Storhaug <s...@shadstorhaug.com>
Subject RE: TestSearcherManager_Mem
Date Fri, 24 Mar 2017 21:34:54 GMT
Vincent,

Thanks for the input.

Yea, I realize how this may sound. But actually, ValueEquals is *primarily* used in tests
and asserts - there might be a few exceptions to that. I created that method as a way to patch
up the testing, totally oblivious to the fact we already had an Array.Equals to fill that
need (although it doesn't match Java's implementation - and it SHOULD). Perhaps there is a
more elegant way to patch up the tests that was overlooked without resorting to this (such
as creating another overload of assertEquals()). But, this is also why I am proposing to factor
it out.

The Arrays.Equals() is (for some odd reason) using the type to distinguish between 2 sequences.
AFAIK, that isn't being done in Java. But like you said, we can't do this blindly, we have
to look at each case one at a time in order to check compatibility (and correctness according
to the Java implementation), and test thoroughly. 

Actually, putting this into the Arrays class feels a bit wrong - since arrays implement IList<T>,
it feels more like this belongs in Collections (which for some odd reason is in a namespace
called Compatibility that I am still trying to figure out a reason for being). Not to mention,
we will need a different implementation for ISet<T> and IDictionary<T>.

But however you look at it, Java's collections compare values by default and .NET's do not.
So, it would be far preferable to have ONE solution (or at least one per collection type)
rather than several. Sure, the tests are passing because they may not assume that the types
of collections being equal matters even though the Equals and GetHashCode implementations
currently do. That doesn't preclude it from being a bug :). 

That said, using a .NETified solution is also correct, and if you look at EquatableList<T>,
they did just that - the only problem is that they weren't looking at the Java source when
they wrote it to be sure it has the right behavior, so it differs and is therefore not a general
solution for Lucene.Net. That is why I created ValueList<T> - I needed a solution that
matched Java that I could use without also potentially breaking everything that depends on
EquatableList<T>. And I hear you - we need to ensure our version calls dispose.

So, I guess we can start by looking at List<T> and Set<T> in Java and making sure
we have exactly the same logic in our Collections.Equals, Collections.GetHashCode, and Collections.ToString().
Then check compatibility and migrate the stuff we can, and if needed, create a different solution
for the stuff that is incompatible (which I am not totally convinced will be required). Alternatively
(or additionally), we could just make a single set of collections that implement the decorator
pattern that override the Equals, GetHashCode, and ToString methods of the wrapped collection
so we can "put a Java-ish glove on a .NET hand" so to speak. 

This is a similar solution I used in some of the Analyzers that use a BufferedReader. In .NET,
it is the underlying stream that determines if it is buffered (or seekable). But I didn't
want to create a messy solution that required you to use a "special" TextReader backed by
a buffered stream just to use the functionality. Nope, instead I ported over BufferedReader
(well, actually it is named BufferedCharFilter) and used it to wrap whatever plain old TextReader
is passed in. Result: we have buffering that some of the Analyzers need (most notably the
HTML one) without having to do anything special in the .NET world. That is what I am thinking
for the collection equality as well - pass in any old collection and get Java-like behavior
inside of Lucene.Net.

I suggest you have a look at AbstractList<T>, AbstractSet<T>, and AbstractMap<K,
V> and see how far we have strayed from the proper implementation by looking at a few examples
in Lucene.Net.Queries and then you might be able to formulate a more informed opinion:

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/util/AbstractList.java#AbstractList.equals%28java.lang.Object%29
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/util/AbstractSet.java#AbstractSet.equals%28java.lang.Object%29
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/util/AbstractMap.java#AbstractMap.equals%28java.lang.Object%29

But yea, sets don't care about order and lists do. Most of the sets, lists, and maps use the
default implementations of these abstract classes. Imagine - in some ways Java is actually
better than .NET!



I don't think there is a reasonable way to port an application the size of Lucene.Net and
make it more .NET like (although I have done my best to stomp out the stench of Java from
the API). You simply cannot diverge very much from the original design if you ever expect
to upgrade to a newer version later. Not to mention, it would not be possible to determine
if you have properly ported it if you had no way to verify it with the original tests (which
often do something so subtle that you don't even have any idea what it is actually testing).
Also, staying close to the original design means that the original documentation is still
pretty useful.

That said, I think that making the API more usable in .NET is certainly possible, and that
is the sales pitch I have been peddling here. For example, the Java designers clearly took
into consideration the fact that you can create anonymous classes in Java in the design. So,
on the Analyzer abstract base class, I created a NewAnonymous() method that uses a delegate
method to simulate this behavior.

Java:

Analyzer analyzer = new Analyzer() {

    @Override
    protected TokenStreamComponents createComponents(String fieldname, Reader reader) {
        Tokenizer source = new FooTokenizer(reader);
        TokenStream filter = new FooFilter(source);
        filter = new BarFilter(filter);
        return new TokenStreamComponents(source, filter);
    }
};

C#:

Analyzer analyzer = Analyzer.NewAnonymous(createComponents: (fieldname, reader) =>
{
    Tokenizer source = new FooTokenizer(reader);
    TokenStream filter = new FooFilter(source);
    filter = new BarFilter(filter);
    return new TokenStreamComponents(source, filter);
};


This prevents you from having to create a new class every time you need to build an analyzer
like in Lucene.Net 3.0.3. I am sure there are many other clever ways you can use extension
methods, delegates, and other cool features of .NET to make the design easier to work with.
But for that, we will need to pool ideas. 

Thanks,
Shad Storhaug (NightOwl888)


-----Original Message-----
From: Van Den Berghe, Vincent [mailto:Vincent.VanDenBerghe@bvdinfo.com] 
Sent: Saturday, March 25, 2017 2:34 AM
To: Shad Storhaug
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Hello Shad,

I'll have to think about what you write more, but I'm a big fan of consolidation. I'll have
to familiarize myself with all the code you're mentioning before making a more coherent statement,
so let me give you my uninformed ".NET view" on the matter for now.

First, too much consolidation is not always good.  For example, in ObjectExtensions I see
this:

        public static bool ValueEquals<T>(this T a, T b)
        {
            if (a is IEnumerable && b is IEnumerable)
            {
                var iter = (b as IEnumerable).GetEnumerator();
                foreach (object value in a as IEnumerable)
                {
                    iter.MoveNext();
                    if (!object.Equals(value, iter.Current))
                    {
                        return false;
                    }
                }
                return true;
            }

            return a.Equals(b);
        }

This makes red flashing lights and claxons go off in my head: no, you cannot enumerate collections
for equality testing. And it's not because the enumerator isn't properly disposed of (another
pet peeve of mine). You can't just compare two enumerable collections, since the first one
may care about the order (e.g. lists), but the other may not (e.g. sets). What exactly constitutes
equality is really whatever makes the code correct.  Remember the curse of Brzozowski!

But for those things that can be consolidated, I agree with "Equatable" as being a better
name than "Value" for collections that can be compared. There are two ways to proceed:
- implement EquatableHashSet<T>, EquatableList<T>, and so on, explicitly implementing
IEquatable<T> with T being the name of the collection.
- implement IEqualityComparer<T> for evert collection T. Conceptually, this is the cleaner
solution (decoupling equality testing from the object implementation is a cleaner solution),
but of course this means that every comparison, dictionary or set needs to be created with
that comparer as parameter. This opens up a whole new class  of bugs: forget one, and you're
hosed.

I don't really know the best way to proceed at this time. Maybe someone else can chime in?

Ever since I looked at the Lucene.net source code, I've been wondering why more time wasn't
invested in making the port more ".NET like". Now that I've experienced the joy of tracking
down bugs and making tests work, I think I understand why <g>.

You're a very courageous person. 

Vincent




-----Original Message-----
From: Shad Storhaug [mailto:shad@shadstorhaug.com] 
Sent: Friday, March 24, 2017 7:18 PM
To: Van Den Berghe, Vincent <Vincent.VanDenBerghe@bvdinfo.com>
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Vincent,

Good find.

Actually, I am thinking that some consolidation is in order:

GETHASHCODE

For comparing collections with GetHashCode() we have:

Support.EquatableList<T>
Support.HashMap<T>
Support.ValueHashSet<T>
Support.ValueList<T>

Plus:

Support.Arrays.GetHashCode()
Support.HashHelpers.GetValueHashode()

These are all doing the same thing (getting a hash code based on a collection), and they are
not all doing it the same way.


EQUALS

Similarly, we have several places that are using Equals() to compare collections:

Support.EquatableList<T>
Support.HashMap<T>
Support.ValueHashSet<T>
Support.ValueList<T>

Plus:

Support.Arrays.Equals()
Support.ObjectExtensions.ValueEquals()

Plus in many places we are using:

ISet<T>.SetEquals()
IEnumerable<T>.SequenceEquals() (LINQ)


My thinking is that we should use one of static helper classes (probably Arrays) to make a
single implementation of each of these, and then override Equals, GetHashCode, and for that
matter ToString() in all of our Support collections and call our helper class in every place.

Not to mention, there are still many Queries (and other such things) that someone decided
to "optimize" by making the implementation consider only the first and last item in the set
(which is a good recipe for mysterious bugs that are nearly impossible to find as far as I
am concerned).

But this is going to take some grace - there are some implementations of GetHashCode and Equals
out there that require a very specific algorithm in order to function correctly.

We should probably just merge Support.EquatableList<T> with Support.ValueList<T>,
(equatable sounds like a more clear name) and rename ValueHashSet to match (or just call it
HashSet<T>, or would that be confusing?). Perhaps there should also be an overload that
accepts IList<T> and ISet<T> in the constructor so we can just wrap existing instances
easily, without having to worry about whether it inherits from our support classes.

Plus all of our support collections should be able to compare their values, just like in Java
- many are missing the overloads. Also we can probably just factor out HashHelpers and ObjectExtensions
altogether.

There are some Queries that have protected access to their clauses list (which originally
was IList<T>, but has been changed to ValueList<T> or EquatableList<T>).
Maybe they should all be reverted back to using IList<T> and the Query itself can call
Arrays.GetHashCode() and Arrays.Equals() static methods to compare internally, without worrying
about whether the list (that might just be any old list) will compare its contents. The only
possible exceptions are where the list is used as a key for a dictionary, but there aren't
many of those. WDYT?

Shad


-----Original Message-----
From: Van Den Berghe, Vincent [mailto:Vincent.VanDenBerghe@bvdinfo.com] 
Sent: Saturday, March 25, 2017 12:37 AM
To: Shad Storhaug
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Hello Shad,

I'd love to give you hope for Brzozowski. The only thing I can give you at this time is the
probable correction of a possible bug.
There's a method in AutomatonTestUtil:

	public static void DeterminizeSimple(Automaton a, ISet<State> initialset)

The second argument of that method is later used as key in a dictionary, like so:

	sets[initialset] = initialset;

This implies that the argument must be a ValueHashSet<State>. 
Now look at this method in AutomatonTestUtil.MinimizeSimple: the calls look like this:

            DeterminizeSimple(a, SpecialOperations.Reverse(a));

This means that SpecialOperations.Reverse(a) must return a ValueHashSet<State>. But
it doesn't: It returns the accepted state as:

            HashSet<State> accept = new HashSet<State>();

I think this needs to be changed in:

            ValueHashSet<State> accept = new ValueHashSet<State>();

This may open another avenue of inquiry. The error syndrome I get from the failed Brzozowski
algorithm are automatons with duplicate states. This may hint at another hash set used as
a key but not created as ValueHashSet

But in any case, I'm quite convinced that SpecialOperations.Reverse should be amended as described.

The investigation continues.

Vincent

-----Original Message-----
From: Shad Storhaug [mailto:shad@shadstorhaug.com] 
Sent: Friday, March 24, 2017 2:04 PM
To: Van Den Berghe, Vincent <Vincent.VanDenBerghe@bvdinfo.com>
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Vincent,

That's great news!

Looks like it is passing always now. But, for some reason in .NET core it is taking MUCH longer
to finish. I am getting about 6 seconds in .NET Framework, and over 1 minute in .NET Core.
Maybe using a stopwatch is the right solution to stabilize this behavior?

To run the tests on .NET Core, open the Lucene.Net.Portable.sln. You may need to run "dotnet
restore" from the command line at the root of the repository in order to get it to compile
(sometimes you have to close and reopen Visual Studio to get the command to take).

I have taken a stab at that IndicNormalizer. It was failing when trying to get a character
out of the BitArray that it previously put in there. But it was designed to work with a Java
BitSet, not a .NET BitArray. Perhaps there is some difference in the way it works that is
causing this (like null character not being stored, or something silly like that). It's a
shot in the dark, but since I cannot get the test to fail under controlled conditions, I have
replaced it with the OpenBitSet (which is basically a Java BitSet with access to its underlying
storage). At the very least, it won't hurt.

I'll also take a closer look at the random "file not closed" failures coming from TestSearcherManager_Mem().
I think you fixed the underlying cause for the main failure. But this is a sign that there
is an unexpected exception being thrown that triggers Dispose() too early. Perhaps there is
still a broken Codec that is causing this failure, which would explain its randomness.

Is there any hope for Brzozowski? I'll make a compromise - if you can solve that one, I will
pretend we are on version 4.9 for TestEarlyTerminationDifferentSorter() so we can put it to
bed - it's probably not worth the effort anyway (I have already spent several days chasing
after that one).

Thanks,
Shad Storhaug (NightOwl888)


-----Original Message-----
From: Van Den Berghe, Vincent [mailto:Vincent.VanDenBerghe@bvdinfo.com] 
Sent: Friday, March 24, 2017 4:40 PM
To: Shad Storhaug
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Hello Shad,

I have a theory about TestCRTReopen: if you look at the java code (https://github.com/apache/lucene-solr/blob/releases/lucene-solr/4.8.0/lucene/core/src/java/org/apache/lucene/search/ControlledRealTimeReopenThread.java),
you see there's a relation between the reentrant lock and its condition variable:

ReentrantLock reopenLock = new ReentrantLock(); Condition reopenCond = reopenLock.newCondition();

Maybe there's some subtlety in there that we miss. The lock is used only as a guard around
the reopen condition, which maybe is how they rule in the Java Shire, but no such concepts
exist as such in C#. 
The closest thing to a "real" ReentrantLock implementation I have ever seen in .NET (complete
with condition variables, fair locking, and so on) is https://github.com/spring-projects/spring-net-threading/blob/master/src/Spring/Spring.Threading/Threading/Locks/ReentrantLock.cs

But that's a gorilla. All we really want is a banana, without the gorilla attached to it.

So that got me thinking: we know what ControlledRealTImeReopenThread does. Why don't we implement
it in "pure" C# instead of trying to translate it from Java using synchronization primitives
that are almost but not quite totally unlike those in .NET? 

You can find the result in the file attached.  I restrained myself and didn't replace Environment.TickCount
with Stopwatch (which would be more correct and guard against overflows that occur in TickCount
every 24.9 days).

In a fit of altruism and insight, I even let all the related unit tests run, and they all
pass.  And finish in time!

But that's in my alternate universe, of course <g>.


Vincent


-----Original Message-----
From: Shad Storhaug [mailto:shad@shadstorhaug.com]
Sent: Friday, March 24, 2017 3:49 AM
To: Van Den Berghe, Vincent <Vincent.VanDenBerghe@bvdinfo.com>
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Vincent,

FYI - TestSearcherManager_Mem() succeeds much more frequently, but still randomly fails.

Also, although I was able to make the error message change for TestCRTReopen(), I didn't manage
to get it working. The problem is pretty obvious - the WaitForGeneration() method (https://github.com/apache/lucenenet/blob/api-work/src/Lucene.Net.Tests/Search/TestControlledRealTimeReopenThread.cs#L680)
is WAY too slow. Even if I increase the wait period from 20 to 60 seconds it still doesn't
finish in time. I played with a few of the variables in ControlledRealTimeReopenThread, but
couldn't get the behavior to change. I verified that PulseAll() gets called, but it doesn't
seem like it is having any effect on the Wait().

For TestEarlyTerminationDifferentSorter(), I reviewed all of the code under test in the Misc
project line by line, but there was nothing significant to fix. So, still broken (sometimes).

Thanks,
Shad Storhaug (NightOwl888)





-----Original Message-----
From: Shad Storhaug [mailto:shad@shadstorhaug.com]
Sent: Thursday, March 23, 2017 8:25 PM
To: Van Den Berghe, Vincent; dev@lucenenet.apache.org
Cc: dev@lucenenet.apache.org
Subject: RE: TestSearcherManager_Mem

Parallel universe or not, I think you are making progress. I found a similar IncrementAndGet
issue in the ThreadedIndexingAndSearchingTestCase that I have already corrected. However,
it only mattered in one case, in all other cases the result of IncrementAndGet was not being
utilized.

It is like someone intentionally wanted to make it impossible to get all of the bugs out of
this code...

Anyway, stupid is as stupid does...I went through and scanned the entire codebase for IncrementAndGet
and compared it against Lucene. Sure enough, the Core.Util.RefCount class was refactored from
its original. I changed it back to the original code (backed by an AtomicInteger/AtomicInt32),
and the TestCRTReopen() test no longer fails almost immediately - after a couple of minutes
it now fails with the message "waited too long for commit generation". I don't know if I fixed
it or broke it more, but it is definitely behaving differently now.

Now, let me see if I can bring your other changes into my universe...perhaps the new failure
has something to do with the reset event.

Thanks,
Shad Storhaug (NightOwl888)


From: Van Den Berghe, Vincent [mailto:Vincent.VanDenBerghe@bvdinfo.com]
Sent: Thursday, March 23, 2017 7:04 PM
To: dev@lucenenet.apache.org
Cc: Shad Storhaug
Subject: TestSearcherManager_Mem

Even though I seem to live in a parallel universe where 42 isn't 42 and 4.8 isn't 4.8, I'll
have a stab at resolving TestSearcherManager_Mem.

First, there is a method in TrackingIndexWriter:

        public virtual long GetAndIncrementGeneration()
        {
            return indexingGen.IncrementAndGet();
        }


The implementation calls the wrong indexGen method: it should call GetAndIncrement(), which
doesn't exist in the .NET version. You can add the method to the AtomicLong class.
Too bad there's no Interlocked.PostIncrement, but it's easy enough:

              public long GetAndIncrement()
              {
                     return Interlocked.Increment(ref value) - 1;
              }

And adjust the call accordingly:

        public virtual long GetAndIncrementGeneration()
        {
            return indexingGen.GetAndIncrement();
        }


Next we turn our attention to ControlledRealTimeReopenThread<T>.
There's an event defined as follows:

        private ManualResetEvent reopenCond = new ManualResetEvent(false);

This is not correct, since the remainder of the implementation only Sets and Waits, but never
resets. Once the event Set, the wait will never ... uh... wait the second time around. Change
this as follows:

        private AutoResetEvent reopenCond = new AutoResetEvent(false);

Next, for some reason, time is counted in nanoseconds, but since Environment.TickCount is
in milliseconds, we need to convert it by multiplying by 1000000.
Unfortunately, this is done by multiplication:

Environment.TickCount * 1000000

Since Environment.TickCount is an int and 1000000 is an int, the result is negative unless
you just rebooted your computer in a Tardis doing a polka backwards.
Define:

                private const long MS_IN_NS = 1000000;

... and change all other references to 1000000 except one (see below) with MS_IN_NS: this
should solve the overflow problem using C#'s promotion rules.
Next, notice that 64-bit integers are sometimes read outside locks:

                searchingGen = refreshStartGen;
                if (targetGen > searchingGen)
                    while (targetGen > searchingGen)


This isn't guaranteed to be atomic, and I'm a curmudgeon when it comes to parallelism and
atomicity. Change all these lines by:

                Interlocked.Exchange(ref searchingGen, refreshStartGen);
                if (targetGen > Interlocked.Read(ref searchingGen))
                    while (targetGen > Interlocked.Read(ref searchingGen))


In my own spacetime continuum, the test now passes.

Added bonus points: dispose of the waitable event in ControlledRealTimeReopenThread<T>
Dispose method by adding:

                                  reopenCond.Dispose();

(after the Monitor.PulseAll(this); statement)

Extreme added bonus points: the following statement is incorrect, but it works anyway:

                  reopenCond.WaitOne(new TimeSpan(sleepNS / 1000000));//Convert NS to Ticks

(the 1000000 should not be replaced by MS_IN_NS in the new version) The reason why it's incorrect
is because the argument to TimeSpan in the call accepts 100-nanoseconds units, and dividing
nanoseconds by 1000000 yields milliseconds instead. So you will wait a delay of a factor 10000
shorter. It turns out that the correction (divide by 100) will cause timeouts in the tests,
so I left it as-is.


But all of this might be wrong, I may not even exists.


Vincent

Mime
View raw message