Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I think whether one uses java.io.* vs java.nio.* or eventually
      java.nio2.*, or some other means, is an under-the-hood implementation
      detail of FSDirectory and doesn't merit a whole separate class.

      I think FSDirectory should be the core class one uses when one's index
      is in the filesystem.

      So, I'd like to deprecate NIOFSDirectory, absorbing it into
      FSDirectory, and add a setting "useNIO" to FSDirectory. It should
      default to "true" for non-Windows OSs, because it gives far better
      concurrent performance on all platforms but Windows (due to known Sun
      JRE issue http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6265734).

      1. LUCENE-1658.patch
        43 kB
        Michael McCandless
      2. LUCENE-1658.patch
        42 kB
        Michael McCandless
      3. LUCENE-1658.patch
        7 kB
        Michael McCandless
      4. LUCENE-1658-take2.patch
        38 kB
        Uwe Schindler
      5. LUCENE-1658-take2.patch
        37 kB
        Uwe Schindler
      6. LUCENE-1658-take3.patch
        44 kB
        Uwe Schindler
      7. LUCENE-1658-take3.patch
        41 kB
        Uwe Schindler
      8. LUCENE-1658-take3.patch
        34 kB
        Uwe Schindler
      9. LUCENE-1658-take3.patch
        27 kB
        Uwe Schindler
      10. LUCENE-1658-take3.patch
        24 kB
        Uwe Schindler
      11. LUCENE-1658-take3.patch
        20 kB
        Uwe Schindler
      12. LUCENE-1658-take3.patch
        20 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          Ok. I did work against trunk, so that explains. Thanks ! (maybe we should start using @since in all future issues, to avoid the confusion)

          Show
          Shai Erera added a comment - Ok. I did work against trunk, so that explains. Thanks ! (maybe we should start using @since in all future issues, to avoid the confusion)
          Hide
          Uwe Schindler added a comment -

          This codeyou are mentioning was not yet released! So before 2.9 there was no ctor for FSDirectory that was public: http://lucene.apache.org/java/2_4_1/api/core/org/apache/lucene/store/FSDirectory.html

          But SimpleFSDirectory can be instantiated. FSDirectory will get abstract in 3.0 and is only the base class and factory anymore.

          Show
          Uwe Schindler added a comment - This codeyou are mentioning was not yet released! So before 2.9 there was no ctor for FSDirectory that was public: http://lucene.apache.org/java/2_4_1/api/core/org/apache/lucene/store/FSDirectory.html But SimpleFSDirectory can be instantiated. FSDirectory will get abstract in 3.0 and is only the base class and factory anymore.
          Hide
          Shai Erera added a comment -

          Uwe - I had code which did new FSDirectory(File, LockFactory), however that ctor is now protected, and my code does not compile. I didn't see any mentions about this in CHANGES. Since this break compat, I think we should either deprecate this ctor (revert it to public), or document in CHANGES under the "changes to back-compat policy". Right?

          Show
          Shai Erera added a comment - Uwe - I had code which did new FSDirectory(File, LockFactory), however that ctor is now protected, and my code does not compile. I didn't see any mentions about this in CHANGES. Since this break compat, I think we should either deprecate this ctor (revert it to public), or document in CHANGES under the "changes to back-compat policy". Right?
          Hide
          Uwe Schindler added a comment -

          Committed revision 780770

          Show
          Uwe Schindler added a comment - Committed revision 780770
          Hide
          Uwe Schindler added a comment -

          hrrrrrm, outdated patch. Missing was the a/b in changes, no fixed.

          Show
          Uwe Schindler added a comment - hrrrrrm, outdated patch. Missing was the a/b in changes, no fixed.
          Hide
          Uwe Schindler added a comment -

          Attached a patch with all changes. I also reworked the exception part in the hack (use the correct PrivilegedException pass-through an init the cause of the IOException, so user knows, if e.g. the security settings were too low).

          I commit shortly.

          Show
          Uwe Schindler added a comment - Attached a patch with all changes. I also reworked the exception part in the hack (use the correct PrivilegedException pass-through an init the cause of the IOException, so user knows, if e.g. the security settings were too low). I commit shortly.
          Hide
          Michael McCandless added a comment -

          bq I reactivated this exception (it was disabled before). In my opinion, we should enable it to notify the user on any problems (e.g. that he may must raise security privileges to enable it correctly). If the user hits an IOException, he can switch off the tweak easily. By the way, switching on the tweak throws IAE if the platform does not support it.

          Yeah I agree it's good to be brittle, so one knows to raise security privileges. But: how intermittent are the exceptions thrown from this code? Is it a situation where it will always fail or always succeed? In which case, I agree we can leave it as it is.

          Show
          Michael McCandless added a comment - bq I reactivated this exception (it was disabled before). In my opinion, we should enable it to notify the user on any problems (e.g. that he may must raise security privileges to enable it correctly). If the user hits an IOException, he can switch off the tweak easily. By the way, switching on the tweak throws IAE if the platform does not support it. Yeah I agree it's good to be brittle, so one knows to raise security privileges. But: how intermittent are the exceptions thrown from this code? Is it a situation where it will always fail or always succeed? In which case, I agree we can leave it as it is.
          Hide
          Uwe Schindler added a comment -

          Maybe don't make MMapDir.cleanUnmapping final, so subclasses could tweak it? I'm still nervous about throwing IOException from that method if the unmap fails, but if we make it non-final then we can leave the IOException as is and users can subclass it if need be.

          I reactivated this exception (it was disabled before). In my opinion, we should enable it to notify the user on any problems (e.g. that he may must raise security privileges to enable it correctly). If the user hits an IOException, he can switch off the tweak easily. By the way, switching on the tweak throws IAE if the platform does not support it.

          cleanUnmapping is currently not really overrideable, because package-private. I can unfinal it and make protected, if needed (I wanted to hide this dangerous method from other usages, not that anyboy calls it for own ByteBuffers and crashes his JVM).

          I will do the other changes, no problem.

          Show
          Uwe Schindler added a comment - Maybe don't make MMapDir.cleanUnmapping final, so subclasses could tweak it? I'm still nervous about throwing IOException from that method if the unmap fails, but if we make it non-final then we can leave the IOException as is and users can subclass it if need be. I reactivated this exception (it was disabled before). In my opinion, we should enable it to notify the user on any problems (e.g. that he may must raise security privileges to enable it correctly). If the user hits an IOException, he can switch off the tweak easily. By the way, switching on the tweak throws IAE if the platform does not support it. cleanUnmapping is currently not really overrideable, because package-private. I can unfinal it and make protected, if needed (I wanted to hide this dangerous method from other usages, not that anyboy calls it for own ByteBuffers and crashes his JVM). I will do the other changes, no problem.
          Hide
          Michael McCandless added a comment -

          Looks good – thanks Uwe – a few small things:

          • Can you remove the a/b from the LUCENE-1658 in CHANGES.txt? (I
            think it may confuse the changes-to-html generation)
          • Can you point out that MMapDirectory will consume transient disk
            space, regardless of platform, because of this sun bug? Ie, this
            bug is not just a "you can't delete the files on Windows"
            problem; it's a problem for unix as well. Maybe something like
            this:
            .
            Due to <a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038">this bug</a>
            in Sun's JRE, MMapDirectory's IndexInput.close is unable to close
            the underlying OS file handle. Only when GC finally collects the
            underlying objects, which could be quite some time later, will the file
            handle be closed.
            .
            This will consume additional transient disk usage: on Windows,
            attempts to delete or overwrite the files will result in an
            exception; on other platforms, which typically have a "delete on
            last close" semantics, while such operations will succeed, the bytes
            are still consuming space on disk. For many applications this
            limitation is not a problem (eg if you have plenty of disk space,
            and you don't rely on overwriting files on Windows) but it's still
            an important limitation to be aware of.
          • Maybe don't make MMapDir.cleanUnmapping final, so subclasses could
            tweak it? I'm still nervous about throwing IOException from that
            method if the unmap fails, but if we make it non-final then we can
            leave the IOException as is and users can subclass it if need be.
          • Can you embed the try/catch inside cleanMapping within { }'s? The
            run-on (if -> try) is confusing now.
          Show
          Michael McCandless added a comment - Looks good – thanks Uwe – a few small things: Can you remove the a/b from the LUCENE-1658 in CHANGES.txt? (I think it may confuse the changes-to-html generation) Can you point out that MMapDirectory will consume transient disk space, regardless of platform, because of this sun bug? Ie, this bug is not just a "you can't delete the files on Windows" problem; it's a problem for unix as well. Maybe something like this: . Due to <a href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038">this bug</a> in Sun's JRE, MMapDirectory's IndexInput.close is unable to close the underlying OS file handle. Only when GC finally collects the underlying objects, which could be quite some time later, will the file handle be closed. . This will consume additional transient disk usage: on Windows, attempts to delete or overwrite the files will result in an exception; on other platforms, which typically have a "delete on last close" semantics, while such operations will succeed, the bytes are still consuming space on disk. For many applications this limitation is not a problem (eg if you have plenty of disk space, and you don't rely on overwriting files on Windows) but it's still an important limitation to be aware of. Maybe don't make MMapDir.cleanUnmapping final, so subclasses could tweak it? I'm still nervous about throwing IOException from that method if the unmap fails, but if we make it non-final then we can leave the IOException as is and users can subclass it if need be. Can you embed the try/catch inside cleanMapping within { }'s? The run-on (if -> try) is confusing now.
          Hide
          Uwe Schindler added a comment -

          New patch with all suggestions, updated Javadocs, additional Ctors with path name only (like static open() method). I also corrected some deprecations and so on. MMapDirectory is no longer returned by open(), the logic is simple now: Windows -> Simple, other -> NIO

          If everybody is fine, I will commit later!

          Show
          Uwe Schindler added a comment - New patch with all suggestions, updated Javadocs, additional Ctors with path name only (like static open() method). I also corrected some deprecations and so on. MMapDirectory is no longer returned by open(), the logic is simple now: Windows -> Simple, other -> NIO If everybody is fine, I will commit later!
          Hide
          Uwe Schindler added a comment -

          I am working on this, because I had the same idea. I added an method setUseUnmap(boolean) and getter to enable this. This method throws IAE if unmap is not supported on the platform.
          I also reenabled the IOException on close(), if there was an error during unmapping.

          Just one additional question: Should I add an additional ctor to the three classes, only taking the dir name, this would then be conformant to FSDir.open()?

          Show
          Uwe Schindler added a comment - I am working on this, because I had the same idea. I added an method setUseUnmap(boolean) and getter to enable this. This method throws IAE if unmap is not supported on the platform. I also reenabled the IOException on close(), if there was an error during unmapping. Just one additional question: Should I add an additional ctor to the three classes, only taking the dir name, this would then be conformant to FSDir.open()?
          Hide
          Michael McCandless added a comment -

          I'm thinking MMapDirectory is too problematic to return by default from open(), because of the unexpected increase transient disk usage.

          I think we should add the hack to MMapDir, disabled by default, and add a setter to enable it (with javadocs clear about the warnings). When used appropriately it can make MMapdir very usable, as a workaround until Sun fixes the bug.

          So I think we should:

          • Fix open to return NIOFSDir on all non-Windows plaforms, and SimpleFSDir on Windows
          • Document the sun bug in MMapDir & FSDir
          • Include hack in MMapDir, disabled by default
          Show
          Michael McCandless added a comment - I'm thinking MMapDirectory is too problematic to return by default from open(), because of the unexpected increase transient disk usage. I think we should add the hack to MMapDir, disabled by default, and add a setter to enable it (with javadocs clear about the warnings). When used appropriately it can make MMapdir very usable, as a workaround until Sun fixes the bug. So I think we should: Fix open to return NIOFSDir on all non-Windows plaforms, and SimpleFSDir on Windows Document the sun bug in MMapDir & FSDir Include hack in MMapDir, disabled by default
          Hide
          Earwin Burrfoot added a comment - - edited

          On a couple of projects I've worked in, they were very reluctant to having packages allocate memory outside the JVM, and that's my understanding of memory mapped buffers.

          mmap does not allocate memory. It allocates address space, and uses the same disk cache system already has.
          For example, you can't cause OOM in your (or another co-existing) app with mmaps (except eating up your own address space on 32bit systems).

          But if you decide to include MMapDir in that auto-create logic, I hope there will be a way to instantiate a specific FSDir, in case we'll have problems with that logic.

          Public constructors for all D variants are a must, and for me they are the best that this patch has to offer

          Show
          Earwin Burrfoot added a comment - - edited On a couple of projects I've worked in, they were very reluctant to having packages allocate memory outside the JVM, and that's my understanding of memory mapped buffers. mmap does not allocate memory. It allocates address space, and uses the same disk cache system already has. For example, you can't cause OOM in your (or another co-existing) app with mmaps (except eating up your own address space on 32bit systems). But if you decide to include MMapDir in that auto-create logic, I hope there will be a way to instantiate a specific FSDir, in case we'll have problems with that logic. Public constructors for all D variants are a must, and for me they are the best that this patch has to offer
          Hide
          Shai Erera added a comment -

          Perhaps I didn't phrase it well. I wasn't worried about memory consumption per sei, but the behavior. On a couple of projects I've worked in, they were very reluctant to having packages allocate memory outside the JVM, and that's my understanding of memory mapped buffers. So what worries me is that by calling FSDir.get()/open(), I will unknowingly do that ...

          While creating SimpleFSDir/NIOFSDir has smaller effect, I think this is not the case with MMapDir. But if you decide to include MMapDir in that auto-create logic, I hope there will be a way to instantiate a specific FSDir, in case we'll have problems with that logic.

          Show
          Shai Erera added a comment - Perhaps I didn't phrase it well. I wasn't worried about memory consumption per sei, but the behavior. On a couple of projects I've worked in, they were very reluctant to having packages allocate memory outside the JVM, and that's my understanding of memory mapped buffers. So what worries me is that by calling FSDir.get()/open(), I will unknowingly do that ... While creating SimpleFSDir/NIOFSDir has smaller effect, I think this is not the case with MMapDir. But if you decide to include MMapDir in that auto-create logic, I hope there will be a way to instantiate a specific FSDir, in case we'll have problems with that logic.
          Hide
          Earwin Burrfoot added a comment -

          I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows.

          It's almost okay for 64bit systems.

          The "hack" also saves transient disk space, on all systems, right?

          That's a nice catch. Now I have some of the non-buggy-but-weird behaviour my app exhibits explained.

          But they have a 64 bit buffer, so you could use it instead of many buffers.

          They don't. When NIO2 project was merged into OpenJDK, they left some stuff unmerged, including 64bit buffers. Currently they aren't present in OpenJDK and Java7 preview builds, and not even a rough estimate is given on whether they are going to make it through, and when.

          Maybe we should move this hack to contrib ( a class that extends MMapDirectory by adding a close method) with a big warning!

          I support this. The hack has some merits if carefully applied, but is outright too dangerous to ship it as default.

          Show
          Earwin Burrfoot added a comment - I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows. It's almost okay for 64bit systems. The "hack" also saves transient disk space, on all systems, right? That's a nice catch. Now I have some of the non-buggy-but-weird behaviour my app exhibits explained. But they have a 64 bit buffer, so you could use it instead of many buffers. They don't. When NIO2 project was merged into OpenJDK, they left some stuff unmerged, including 64bit buffers. Currently they aren't present in OpenJDK and Java7 preview builds, and not even a rough estimate is given on whether they are going to make it through, and when. Maybe we should move this hack to contrib ( a class that extends MMapDirectory by adding a close method) with a big warning! I support this. The hack has some merits if carefully applied, but is outright too dangerous to ship it as default.
          Hide
          Uwe Schindler added a comment -

          I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows. Imagine a large index which is suddenly mapped to MMB w/o anyone intending to do so.

          This would not be a problem, because MMaped files does not "consume" real memory. If a part of the index is seen in "real memory" still is in the responsibility of the operating system. I have a mmaped index here that is updated very often. It leads to a lot of assigned address space in top for the process but does not affect system performance or swapping (sometimes beyond the physical limit of the machine (mem + swap). Memory mapping files does not consume RAM, it just consumes address space (this is why I changed the javadocs a little bit to clarify this).

          The "hack" also saves transient disk space, on all systems, right? Just because the file name link is deleted on Unix, the bytes are still consuming disk space.

          Yes, this is why I let the fix enabled for any OS.

          Also: I think we should fix the open logic so that on an unknown platform (not unix, not windows, not 64 bit), return SimpleFSDir?

          I would not do this.

          NIO2 still has no solution for the problem. But they have a 64 bit buffer, so you could use it instead of many buffers. On the Amsterdam ApacheCon conference, this was part of the discussion with the guy from sun, but I forgot what the conclusion was! Maybe Oracle helps here

          Maybe we should move this hack to contrib ( a class that extends MMapDirectory by adding a close method) with a big warning!

          Show
          Uwe Schindler added a comment - I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows. Imagine a large index which is suddenly mapped to MMB w/o anyone intending to do so. This would not be a problem, because MMaped files does not "consume" real memory. If a part of the index is seen in "real memory" still is in the responsibility of the operating system. I have a mmaped index here that is updated very often. It leads to a lot of assigned address space in top for the process but does not affect system performance or swapping (sometimes beyond the physical limit of the machine (mem + swap). Memory mapping files does not consume RAM, it just consumes address space (this is why I changed the javadocs a little bit to clarify this). The "hack" also saves transient disk space, on all systems, right? Just because the file name link is deleted on Unix, the bytes are still consuming disk space. Yes, this is why I let the fix enabled for any OS. Also: I think we should fix the open logic so that on an unknown platform (not unix, not windows, not 64 bit), return SimpleFSDir? I would not do this. NIO2 still has no solution for the problem. But they have a 64 bit buffer, so you could use it instead of many buffers. On the Amsterdam ApacheCon conference, this was part of the discussion with the guy from sun, but I forgot what the conclusion was! Maybe Oracle helps here Maybe we should move this hack to contrib ( a class that extends MMapDirectory by adding a close method) with a big warning!
          Hide
          Michael McCandless added a comment -

          I removed the test completely here, but forgot to include this into the patch.

          OK that makes sense. When you commit this, you can also just remove the test from the back-compat branch...

          The "hack" also saves transient disk space, on all systems, right? Just because the file name link is deleted on Unix, the bytes are still consuming disk space. So, Uwe in javadoc'ing Sun's bug, can you note that the bug causes higher transient disk usage? Maybe we all should go vote for the bug... but it seems our votes will be in the noise. I'll go add this bug to http://wiki.apache.org/lucene-java/SunJavaBugs

          Show
          Michael McCandless added a comment - I removed the test completely here, but forgot to include this into the patch. OK that makes sense. When you commit this, you can also just remove the test from the back-compat branch... The "hack" also saves transient disk space, on all systems, right? Just because the file name link is deleted on Unix, the bytes are still consuming disk space. So, Uwe in javadoc'ing Sun's bug, can you note that the bug causes higher transient disk usage? Maybe we all should go vote for the bug... but it seems our votes will be in the noise. I'll go add this bug to http://wiki.apache.org/lucene-java/SunJavaBugs
          Hide
          Shai Erera added a comment -

          I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows. Imagine a large index which is suddenly mapped to MMB w/o anyone intending to do so.

          I think that because of the memory implications of using MMapDir we should let apps explicitly create it, and not under the covers, because of OS parameter. I think that using MMapDir goes far beyond just OS, and hence why it's dangerous to create it under the covers.

          Show
          Shai Erera added a comment - I'm a bit nervous about creating MMapDirectory automatically for any OS, not just Windows. Imagine a large index which is suddenly mapped to MMB w/o anyone intending to do so. I think that because of the memory implications of using MMapDir we should let apps explicitly create it, and not under the covers, because of OS parameter. I think that using MMapDir goes far beyond just OS, and hence why it's dangerous to create it under the covers.
          Hide
          Michael McCandless added a comment -

          OK the hack now scares me!

          Let's just return SimpleFSDir on all Windows? And, note this bug in the FSDir & MMapDir javadocs.

          Also: I think we should fix the open logic so that on an unknown platform (not unix, not windows, not 64 bit), return SimpleFSDir?

          Show
          Michael McCandless added a comment - OK the hack now scares me! Let's just return SimpleFSDir on all Windows? And, note this bug in the FSDir & MMapDir javadocs. Also: I think we should fix the open logic so that on an unknown platform (not unix, not windows, not 64 bit), return SimpleFSDir?
          Hide
          Uwe Schindler added a comment -

          You are right, it is quite dangerous, if somebody e.g. closes an IndexReader in one thread and another thread still accesses it (this happens often).

          Show
          Uwe Schindler added a comment - You are right, it is quite dangerous, if somebody e.g. closes an IndexReader in one thread and another thread still accesses it (this happens often).
          Hide
          Earwin Burrfoot added a comment -

          It uses less virtual memory

          64bit systems have an abundance of said valuable resource. Why taint them with dangerous hacks for the sake of zero returns?

          Show
          Earwin Burrfoot added a comment - It uses less virtual memory 64bit systems have an abundance of said valuable resource. Why taint them with dangerous hacks for the sake of zero returns?
          Hide
          Earwin Burrfoot added a comment -

          I tested on MacOS:

          Invalid memory access of location 8b55a000 rip=0110c367

          • Here JVM quietly dies. non-null return code, all threads are killed, no diagnostic files created.
          Show
          Earwin Burrfoot added a comment - I tested on MacOS: Invalid memory access of location 8b55a000 rip=0110c367 Here JVM quietly dies. non-null return code, all threads are killed, no diagnostic files created.
          Hide
          Uwe Schindler added a comment -

          You also employ the hack on non-windows machines, that work quite well without it. What for?

          It uses less virtual memory

          Ah! You was referring to your code. It's not thread-safe still. Someone could access the closed buffer before it sees the now-null reference to it.

          For the thread-safety, youre correct. To solve this, we have to add the synchronized isClosed check on our side, which kills performance. I wanted to check, what happens, if somebody really accesses the buffer after unmapping, does it crash the JVM?

          Show
          Uwe Schindler added a comment - You also employ the hack on non-windows machines, that work quite well without it. What for? It uses less virtual memory Ah! You was referring to your code. It's not thread-safe still. Someone could access the closed buffer before it sees the now-null reference to it. For the thread-safety, youre correct. To solve this, we have to add the synchronized isClosed check on our side, which kills performance. I wanted to check, what happens, if somebody really accesses the buffer after unmapping, does it crash the JVM?
          Hide
          Uwe Schindler added a comment -

          I know this code , its on every platform the same. For our MMapDirectory this is not a problem, as nobody can accidently read/write the buffer, because the reference to it is nulled in our code. The buffer member itsself is private, so nobody can access it from outside. So after closing the IndexInput, nothing can access the buffer.
          This is what I meant with "nulling".

          Show
          Uwe Schindler added a comment - I know this code , its on every platform the same. For our MMapDirectory this is not a problem, as nobody can accidently read/write the buffer, because the reference to it is nulled in our code. The buffer member itsself is private, so nobody can access it from outside. So after closing the IndexInput, nothing can access the buffer. This is what I meant with "nulling".
          Hide
          Earwin Burrfoot added a comment -

          Ah! You was referring to your code. It's not thread-safe still. Someone could access the closed buffer before it sees the now-null reference to it.
          You also employ the hack on non-windows machines, that work quite well without it. What for?

          Show
          Earwin Burrfoot added a comment - Ah! You was referring to your code. It's not thread-safe still. Someone could access the closed buffer before it sees the now-null reference to it. You also employ the hack on non-windows machines, that work quite well without it. What for?
          Hide
          Uwe Schindler added a comment -

          Updated patch: I forgot to add a check, if the IndexInput was already closed.

          Show
          Uwe Schindler added a comment - Updated patch: I forgot to add a check, if the IndexInput was already closed.
          Hide
          Earwin Burrfoot added a comment - - edited

          The buffer is nulled directly after unmapping.

          Really? Let me quote some code (MacOS, Java 1.6):

          unsafe.freeMemory(address);
          address = 0;
          Bits.unreserveMemory(capacity);

          Does windows version differ? What we see here is 'zeroing', not 'nulling'. When doing get/set, buffer never checks for address to have sense, so the next access will yield a GPF
          The guys from Sun explained the absence of unmap() in the original design - the only way of closing mapped buffer and not getting unpredictable behaviour is to introduce a synchronized isClosed check on each read/write operation, which kills the performance even if the sync method used is just a volatile variable.

          Show
          Earwin Burrfoot added a comment - - edited The buffer is nulled directly after unmapping. Really? Let me quote some code (MacOS, Java 1.6): unsafe.freeMemory(address); address = 0; Bits.unreserveMemory(capacity); Does windows version differ? What we see here is 'zeroing', not 'nulling'. When doing get/set, buffer never checks for address to have sense, so the next access will yield a GPF The guys from Sun explained the absence of unmap() in the original design - the only way of closing mapped buffer and not getting unpredictable behaviour is to introduce a synchronized isClosed check on each read/write operation, which kills the performance even if the sync method used is just a volatile variable.
          Hide
          Uwe Schindler added a comment -

          Here the patch with conditionalized MMapDirectory on windows. I am a bit nervous...
          We should discuss it a little bit and commit it. But everybody should test it extensive before releasing!
          Users still using FSDir.getDirectory would not be affected, so it would not break existing apps.

          Show
          Uwe Schindler added a comment - Here the patch with conditionalized MMapDirectory on windows. I am a bit nervous... We should discuss it a little bit and commit it. But everybody should test it extensive before releasing! Users still using FSDir.getDirectory would not be affected, so it would not break existing apps.
          Hide
          Uwe Schindler added a comment -

          The buffer is nulled directly after unmapping.

          Show
          Uwe Schindler added a comment - The buffer is nulled directly after unmapping.
          Hide
          Earwin Burrfoot added a comment -

          I told you, Java mmap doesn't work on Windows.
          And please, don't use the unmap hack! If it doesn't work, it doesn't work. Let's for all windows versions use SimpleFSD.
          Look, what are you going to do if you unmap a buffer and then access it by accident? Crash JVM?

          Show
          Earwin Burrfoot added a comment - I told you, Java mmap doesn't work on Windows. And please, don't use the unmap hack! If it doesn't work, it doesn't work. Let's for all windows versions use SimpleFSD. Look, what are you going to do if you unmap a buffer and then access it by accident? Crash JVM?
          Hide
          Uwe Schindler added a comment -

          I think we should fix the test to make an exception...

          I removed the test completely here, but forgot to include this into the patch. The test is now invalid with the new class hierarchy of FSDir. If FSDir gets abstract in 3.0, it is clear which methods need to be overridden and so on. The intention behind the test was, to check, if MMap does not overwrite all FSDir Methods, and because of this one of the calls could falsely return the standard FSDir impl of IndexInput. After Lucene 3.0, this is not possible, as MMapDir does not extend SimpleFSDir.

          If we remove the test and then backwards-tag tests fail because of this, we could easily fix this by adding the method, simply calling super(). Or remove the test in backwards-tag, too.

          But: I think we shouldn't throw an exception if the hack fails? Ie, just fallback to relying on GC to eventually unmap? (What MMapDir does today).

          And then we can make the default on Windows 64 be MMapDir again?

          Also, can you reference this bug from FSDir's/MMapDir's javadocs?

          Good idea. The problem is, if the hack fails, IndexReader may suddenly fail later on windows to delete files. Or was this only a problem in the tests. What happens, if e.g. IndexWriter wants to write a new segments.gen file and so on, but it is still mmapped? So I preferred to throw the IOException, but I can disable this easily.

          I would suggest to check on windows, if class sun.misc.Cleaner is available and if not, return SimpleFSDir. For Unix, it is enough to simply ignore the cleanup (throw no exception).

          Show
          Uwe Schindler added a comment - I think we should fix the test to make an exception... I removed the test completely here, but forgot to include this into the patch. The test is now invalid with the new class hierarchy of FSDir. If FSDir gets abstract in 3.0, it is clear which methods need to be overridden and so on. The intention behind the test was, to check, if MMap does not overwrite all FSDir Methods, and because of this one of the calls could falsely return the standard FSDir impl of IndexInput. After Lucene 3.0, this is not possible, as MMapDir does not extend SimpleFSDir. If we remove the test and then backwards-tag tests fail because of this, we could easily fix this by adding the method, simply calling super(). Or remove the test in backwards-tag, too. But: I think we shouldn't throw an exception if the hack fails? Ie, just fallback to relying on GC to eventually unmap? (What MMapDir does today). And then we can make the default on Windows 64 be MMapDir again? Also, can you reference this bug from FSDir's/MMapDir's javadocs? Good idea. The problem is, if the hack fails, IndexReader may suddenly fail later on windows to delete files. Or was this only a problem in the tests. What happens, if e.g. IndexWriter wants to write a new segments.gen file and so on, but it is still mmapped? So I preferred to throw the IOException, but I can disable this easily. I would suggest to check on windows, if class sun.misc.Cleaner is available and if not, return SimpleFSDir. For Unix, it is enough to simply ignore the cleanup (throw no exception).
          Hide
          Michael McCandless added a comment -

          I'm seeing this failure:

          [junit] Testcase: testIndexInputMethods(org.apache.lucene.store.TestMMapDirectory): FAILED
          [junit] FSDirectory has method public org.apache.lucene.store.IndexInput org.apache.lucene.store.FSDirectory.openInput(java.lang.String) throws java.io.IOException but MMapDirectory does not override
          [junit] junit.framework.AssertionFailedError: FSDirectory has method public org.apache.lucene.store.IndexInput org.apache.lucene.store.FSDirectory.openInput(java.lang.String) throws java.io.IOException but MMapDirectory does not override
          [junit] at org.apache.lucene.store.TestMMapDirectory.testIndexInputMethods(TestMMapDirectory.java:43)
          [junit] at org.apache.lucene.util.LuceneTestCase.runTest(LuceneTestCase.java:88)

          I think we should fix the test to make an exception...

          Show
          Michael McCandless added a comment - I'm seeing this failure: [junit] Testcase: testIndexInputMethods(org.apache.lucene.store.TestMMapDirectory): FAILED [junit] FSDirectory has method public org.apache.lucene.store.IndexInput org.apache.lucene.store.FSDirectory.openInput(java.lang.String) throws java.io.IOException but MMapDirectory does not override [junit] junit.framework.AssertionFailedError: FSDirectory has method public org.apache.lucene.store.IndexInput org.apache.lucene.store.FSDirectory.openInput(java.lang.String) throws java.io.IOException but MMapDirectory does not override [junit] at org.apache.lucene.store.TestMMapDirectory.testIndexInputMethods(TestMMapDirectory.java:43) [junit] at org.apache.lucene.util.LuceneTestCase.runTest(LuceneTestCase.java:88) I think we should fix the test to make an exception...
          Hide
          Michael McCandless added a comment -

          All tests pass now JUHU!

          Excellent!

          It may fail on specific non-Sun VMs and may hit SecurityExceptions. If this happens, the close() call will throw an IOException.

          I think we should do the hack. It seems better than nothing.

          But: I think we shouldn't throw an exception if the hack fails? Ie, just fallback to relying on GC to eventually unmap? (What MMapDir does today).

          And then we can make the default on Windows 64 be MMapDir again?

          Also, can you reference this bug from FSDir's/MMapDir's javadocs?

          Show
          Michael McCandless added a comment - All tests pass now JUHU! Excellent! It may fail on specific non-Sun VMs and may hit SecurityExceptions. If this happens, the close() call will throw an IOException. I think we should do the hack. It seems better than nothing. But: I think we shouldn't throw an exception if the hack fails? Ie, just fallback to relying on GC to eventually unmap? (What MMapDir does today). And then we can make the default on Windows 64 be MMapDir again? Also, can you reference this bug from FSDir's/MMapDir's javadocs?
          Hide
          Uwe Schindler added a comment -

          Some cleanups with initOutput (remove duplicate code) and unneeded methods in MMapDir.
          All tests pass. Will try tomorrow also on Solaris x64 with MMap.

          Show
          Uwe Schindler added a comment - Some cleanups with initOutput (remove duplicate code) and unneeded methods in MMapDir. All tests pass. Will try tomorrow also on Solaris x64 with MMap.
          Hide
          Uwe Schindler added a comment -

          Atached is a patch that now works (at least on windows) for all three versions of FSDir. One additional test was fixed to SimpleFSDir (because it assumesan BufferedIndexInput).

          This patch contains the tweaked MMapDir that has the following features:

          • Throws correct IOExceptions on read past EOF
          • Is able to unmap the buffer when close is called (for cloned inputs nothing is done, this is similar to other FSDirs). This unmapping is an "illegal and unsecure hack" according to Sun, but I have seen other open source projects, that use it.

          The problems with unmapping are: It may fail on specific non-Sun VMs and may hit SecurityExceptions. If this happens, the close() call will throw an IOException. The good thing is: The virtual memory usage is lower and with small indexes, the 32 bit VMs do not hit OOMs, if buffers are not unmapped by GC early.

          What do you think? Should we supply this "extended" MMapDirectory?
          Earwin, did you try this, too?

          All tests pass now JUHU!

          Show
          Uwe Schindler added a comment - Atached is a patch that now works (at least on windows) for all three versions of FSDir. One additional test was fixed to SimpleFSDir (because it assumesan BufferedIndexInput). This patch contains the tweaked MMapDir that has the following features: Throws correct IOExceptions on read past EOF Is able to unmap the buffer when close is called (for cloned inputs nothing is done, this is similar to other FSDirs). This unmapping is an "illegal and unsecure hack" according to Sun, but I have seen other open source projects, that use it. The problems with unmapping are: It may fail on specific non-Sun VMs and may hit SecurityExceptions. If this happens, the close() call will throw an IOException. The good thing is: The virtual memory usage is lower and with small indexes, the 32 bit VMs do not hit OOMs, if buffers are not unmapped by GC early. What do you think? Should we supply this "extended" MMapDirectory? Earwin, did you try this, too? All tests pass now JUHU!
          Hide
          Uwe Schindler added a comment -

          I dug down and found that this test illegally opens SegmentReaders on files that IndexWriter still has open for writing, and somehow this causes problems when using an MMapDir. I'll open a separate issue and

          put details there.

          This is a NIO bug in windows, I assume. In Google I found a report at sun about this, too. Mapped buffers from UNC-pathes have wrong bytes in their buffer.

          By the way, the other failing tests are easy to fix:
          Some tests check, if the IndexInput throws an IOException when reading past eof. When doing this with a Byte buffer, the get() throws an BufferUnderflowException. It can be fixed like this in MMapIndexInputs:

              public byte readByte() throws IOException {
                try {
                  return buffer.get();
                } catch (BufferUnderflowException e) {
                  throw new IOException("read past eof");
                }
              }
          

          The other failures are harmless, but it would be good to fix them. I am working on that and then test extensive.

          The problem with not freeing the buffer can be fixed on windows using the bad hack with this sun.misc.Cleaner class and PrivilegedAction (described in the bug report), but this depends on Sun's internals and works only with Sun's JRE. And it may fail on some under-priviledged environments like web containers.

          But nevertheless, with this bad hack, my local version works now without any failing test on Win32 using MMap.

          Show
          Uwe Schindler added a comment - I dug down and found that this test illegally opens SegmentReaders on files that IndexWriter still has open for writing, and somehow this causes problems when using an MMapDir. I'll open a separate issue and put details there. This is a NIO bug in windows, I assume. In Google I found a report at sun about this, too. Mapped buffers from UNC-pathes have wrong bytes in their buffer. By the way, the other failing tests are easy to fix: Some tests check, if the IndexInput throws an IOException when reading past eof. When doing this with a Byte buffer, the get() throws an BufferUnderflowException. It can be fixed like this in MMapIndexInputs: public byte readByte() throws IOException { try { return buffer.get(); } catch (BufferUnderflowException e) { throw new IOException( "read past eof" ); } } The other failures are harmless, but it would be good to fix them. I am working on that and then test extensive. The problem with not freeing the buffer can be fixed on windows using the bad hack with this sun.misc.Cleaner class and PrivilegedAction (described in the bug report), but this depends on Sun's internals and works only with Sun's JRE. And it may fail on some under-priviledged environments like web containers. But nevertheless, with this bad hack, my local version works now without any failing test on Win32 using MMap.
          Hide
          Michael McCandless added a comment -

          a very lot of tests are failing with MMap.

          It looks like all but one (see below) of the failures are "innocent",
          eg the test hits an exception while cleaning up (removing the index
          directory it had created), or the test is trying to overwrite a file.

          I agree this will be a hassle for normal usage of Lucene, so let's
          change the default on Win64 to SimpleFSDir (can't be NIOFSDir because
          of another Sun bug). So, for open(), on non-Windows 32 bit we use
          NIOFSDir, on non-Windows 64 bit we use MMapDir, and on Windows 32 or
          64 we use SimpleFSDir. Crazy how many challenges there are with IO on
          Windows from Java...

          This is the problem on windows: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038

          Sheesh that's alot of votes!

          Can you add that bug into the toplevel javadocs of FSDir explaining
          why MMapDir is not a great choice on even Win64?

          There was one odd failure I noticed, on Win64 when running tests from
          a mounted remote (CIFS) drive:

              [junit] Testcase: testIndexAndMerge(org.apache.lucene.index.TestDoc):	FAILED
              [junit] junit.framework.AssertionFailedError:
              [junit] 	at org.apache.lucene.index.FieldsWriter.addRawDocuments(FieldsWriter.java:249)
              [junit] 	at org.apache.lucene.index.SegmentMerger.mergeFields(SegmentMerger.java:350)
              [junit] 	at org.apache.lucene.index.SegmentMerger.merge(SegmentMerger.java:139)
              [junit] 	at org.apache.lucene.index.SegmentMerger.merge(SegmentMerger.java:116)
              [junit] 	at org.apache.lucene.index.TestDoc.merge(TestDoc.java:182)
              [junit] 	at org.apache.lucene.index.TestDoc.testIndexAndMerge(TestDoc.java:117)
              [junit] 	at org.apache.lucene.util.LuceneTestCase.runTest(LuceneTestCase.java:88)
          

          I dug down and found that this test illegally opens SegmentReaders on
          files that IndexWriter still has open for writing, and somehow this
          causes problems when using an MMapDir. I'll open a separate issue and
          put details there.

          Show
          Michael McCandless added a comment - a very lot of tests are failing with MMap. It looks like all but one (see below) of the failures are "innocent", eg the test hits an exception while cleaning up (removing the index directory it had created), or the test is trying to overwrite a file. I agree this will be a hassle for normal usage of Lucene, so let's change the default on Win64 to SimpleFSDir (can't be NIOFSDir because of another Sun bug). So, for open(), on non-Windows 32 bit we use NIOFSDir, on non-Windows 64 bit we use MMapDir, and on Windows 32 or 64 we use SimpleFSDir. Crazy how many challenges there are with IO on Windows from Java... This is the problem on windows: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038 Sheesh that's alot of votes! Can you add that bug into the toplevel javadocs of FSDir explaining why MMapDir is not a great choice on even Win64? There was one odd failure I noticed, on Win64 when running tests from a mounted remote (CIFS) drive: [junit] Testcase: testIndexAndMerge(org.apache.lucene.index.TestDoc): FAILED [junit] junit.framework.AssertionFailedError: [junit] at org.apache.lucene.index.FieldsWriter.addRawDocuments(FieldsWriter.java:249) [junit] at org.apache.lucene.index.SegmentMerger.mergeFields(SegmentMerger.java:350) [junit] at org.apache.lucene.index.SegmentMerger.merge(SegmentMerger.java:139) [junit] at org.apache.lucene.index.SegmentMerger.merge(SegmentMerger.java:116) [junit] at org.apache.lucene.index.TestDoc.merge(TestDoc.java:182) [junit] at org.apache.lucene.index.TestDoc.testIndexAndMerge(TestDoc.java:117) [junit] at org.apache.lucene.util.LuceneTestCase.runTest(LuceneTestCase.java:88) I dug down and found that this test illegally opens SegmentReaders on files that IndexWriter still has open for writing, and somehow this causes problems when using an MMapDir. I'll open a separate issue and put details there.
          Hide
          Uwe Schindler added a comment - - edited

          Oh shit, on windows, a very lot of tests are failing with MMap. Windows says, you cannot delete or modify files, which have a mapping, with Solaris it works without problems:
          e.g. TestAtomicUpdates:
          [junit] C:\Projects\lucene\trunk\build\test\19.cfs_12.cfs (Der Vorgang ist
          bei einer Datei mit einem geöffneten Bereich, der einem Benutzer zugeordnet ist,
          nicht anwendbar)

          This is the problem on windows: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038

          The problem is, that a memory mapped area is not released on close() in Java, it is released, when GC frees it.
          So the problem may be that MMap can only be used for reading indexes (so only read-only IndexReaders) on windows.

          What should we do, again disable MMap and only use NIOFSDir?

          Show
          Uwe Schindler added a comment - - edited Oh shit, on windows, a very lot of tests are failing with MMap. Windows says, you cannot delete or modify files, which have a mapping, with Solaris it works without problems: e.g. TestAtomicUpdates: [junit] C:\Projects\lucene\trunk\build\test\19.cfs_12.cfs (Der Vorgang ist bei einer Datei mit einem geöffneten Bereich, der einem Benutzer zugeordnet ist, nicht anwendbar) This is the problem on windows: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4724038 The problem is, that a memory mapped area is not released on close() in Java, it is released, when GC frees it. So the problem may be that MMap can only be used for reading indexes (so only read-only IndexReaders) on windows. What should we do, again disable MMap and only use NIOFSDir?
          Hide
          Uwe Schindler added a comment -

          I think I forgot them when I reverted my changes. Currently I am running the tests again with MMap and after that with NIO.

          Show
          Uwe Schindler added a comment - I think I forgot them when I reverted my changes. Currently I am running the tests again with MMap and after that with NIO.
          Hide
          Michael McCandless added a comment -

          Patch looks good, thanks Uwe!

          I still see failures when I use MMapDir (some tests are assuming they'll always get an FSDir).

          I can tackle these if you haven't already?

          Show
          Michael McCandless added a comment - Patch looks good, thanks Uwe! I still see failures when I use MMapDir (some tests are assuming they'll always get an FSDir). I can tackle these if you haven't already?
          Hide
          Uwe Schindler added a comment -

          Previous patch missed correct ctor for SimpleFSDir for reflection.

          Show
          Uwe Schindler added a comment - Previous patch missed correct ctor for SimpleFSDir for reflection.
          Hide
          Uwe Schindler added a comment -

          This patch fixes the failing tests and contains the other improvements.

          Show
          Uwe Schindler added a comment - This patch fixes the failing tests and contains the other improvements.
          Hide
          Uwe Schindler added a comment -

          To conclude:
          The error, I found is only relevant, if the string-type dir arguments to IndexReader are passed to FSDir.open() and are separate instances without refcounting. The reopen code sometimes closes the directory, although it should not, which leads to problems, if the directories are separate instances.
          With cached dirs, its no problem. So if we deprecate the string directory arguments and inside that methods let FSDir.getDirectory() stay alive, we have no problem. In 3.0, one could then remove all this code like refcounting/changing dirs and the everywhere arguments closeDirectory in SegmentReader/DirectoryIndexReader/MultiSegmentReader/... Then 1453 is completely solved.

          Show
          Uwe Schindler added a comment - To conclude: The error, I found is only relevant, if the string-type dir arguments to IndexReader are passed to FSDir.open() and are separate instances without refcounting. The reopen code sometimes closes the directory, although it should not, which leads to problems, if the directories are separate instances. With cached dirs, its no problem. So if we deprecate the string directory arguments and inside that methods let FSDir.getDirectory() stay alive, we have no problem. In 3.0, one could then remove all this code like refcounting/changing dirs and the everywhere arguments closeDirectory in SegmentReader/DirectoryIndexReader/MultiSegmentReader/... Then 1453 is completely solved.
          Hide
          Uwe Schindler added a comment -

          Yikes - you mean there's a pre-existing issue with the fix in LUCENE-1453, before this issue was committed? I agree that code is very spooky, and I'll be very happy once we remove (in 3.0) the cache/refCounting done by FSDir.getDirectory....

          The problem is not really the caching/refcounting. The reopen code sometimes seems to close the directory, even than it should not. There is no difference if the directory is cached (and the number of opens() is tracked by refCount) or standalone. The problem is that SegmentReader's readNorms() hits AlreadyClosedException then...

          The interesting thing is that ou can more easy reproduce it with stand-alone dirs (because I removed the caching, you know). The 1453 fix is also needed for stand-alone dirs (so everytime codeDirectory=true), because, if they are closed during an error/whatever, the same happens. It does not happen so easy with cached dirs, because the old reader is normally closed after the new reopened one, so during the reopen, the refcount is big enough.

          In my opinion, the only good solution is to remove the whole directory-close stuff from readers/writers in 3.0, as said before. And this can be done by removing the string-type dir arguments from Reader/Writer. And: As these use currently FSDir.getDirectory() they should be changed to FSDir.open() or deprecated, I tend to the last one.

          Show
          Uwe Schindler added a comment - Yikes - you mean there's a pre-existing issue with the fix in LUCENE-1453 , before this issue was committed? I agree that code is very spooky, and I'll be very happy once we remove (in 3.0) the cache/refCounting done by FSDir.getDirectory.... The problem is not really the caching/refcounting. The reopen code sometimes seems to close the directory, even than it should not. There is no difference if the directory is cached (and the number of opens() is tracked by refCount) or standalone. The problem is that SegmentReader's readNorms() hits AlreadyClosedException then... The interesting thing is that ou can more easy reproduce it with stand-alone dirs (because I removed the caching, you know). The 1453 fix is also needed for stand-alone dirs (so everytime codeDirectory=true), because, if they are closed during an error/whatever, the same happens. It does not happen so easy with cached dirs, because the old reader is normally closed after the new reopened one, so during the reopen, the refcount is big enough. In my opinion, the only good solution is to remove the whole directory-close stuff from readers/writers in 3.0, as said before. And this can be done by removing the string-type dir arguments from Reader/Writer. And: As these use currently FSDir.getDirectory() they should be changed to FSDir.open() or deprecated, I tend to the last one.
          Hide
          Michael McCandless added a comment -

          I am not so happy , but I work on it.

          I am looking now since 5 hourcs and cannot find the error, the code consists of a lot of printlns and so on.

          We can tag-team if you want Let me know if you want me to take over...

          This failure depends on the random seed in testFSDirectoryReopen2() and is reproducible also with the current version of FSDirectory. It seems that 1453 is not completely fixed.

          Yikes – you mean there's a pre-existing issue with the fix in LUCENE-1453, before this issue was committed? I agree that code is very spooky, and I'll be very happy once we remove (in 3.0) the cache/refCounting done by FSDir.getDirectory....

          If we do not want to do this, I suggest to deprecate all these methods and tell the user to open the directory themselfes and pass Directory instances to Writer/Reader

          I like this (deprecating the open methods that take File/String) best!

          I have a lot of other fixes for the failing tests on some platforms, I will post a revised patch some time with getDirectory deprecated and open() again.

          OK thanks.

          Show
          Michael McCandless added a comment - I am not so happy , but I work on it. I am looking now since 5 hourcs and cannot find the error, the code consists of a lot of printlns and so on. We can tag-team if you want Let me know if you want me to take over... This failure depends on the random seed in testFSDirectoryReopen2() and is reproducible also with the current version of FSDirectory. It seems that 1453 is not completely fixed. Yikes – you mean there's a pre-existing issue with the fix in LUCENE-1453 , before this issue was committed? I agree that code is very spooky, and I'll be very happy once we remove (in 3.0) the cache/refCounting done by FSDir.getDirectory.... If we do not want to do this, I suggest to deprecate all these methods and tell the user to open the directory themselfes and pass Directory instances to Writer/Reader I like this (deprecating the open methods that take File/String) best! I have a lot of other fixes for the failing tests on some platforms, I will post a revised patch some time with getDirectory deprecated and open() again. OK thanks.
          Hide
          Uwe Schindler added a comment - - edited

          I am not so happy , but I work on it.

          Currently my problem is more a failure for the test in LUCENE-1453. After reopening a DirectoryIndexReader, sometimes (not always) the directory is closed, even with the 1453-workaround.

          This failure depends on the random seed in testFSDirectoryReopen2() and is reproducible also with the current version of FSDirectory. It seems that 1453 is not completely fixed. I am looking now since 5 hourcs and cannot find the error, the code consists of a lot of printlns and so on.

          When I found out, what the problem is, I will perhaps open an additional issue, but it seems, that the problem has to do with the FSDirectory changes. Somewhere the directory is closed, although it should be stay open. But what I can say: The error is simplier to reproduce, if the directory is not cached!

          I have a lot of other fixes for the failing tests on some platforms, I will post a revised patch some time with getDirectory deprecated and open() again.

          One thing: For investigating this bug, I changed the IndexReader.open() methods to use a non-cached directory. I think we should do this in the final version, too (also for Indexwriter). So also replace FSDir.getDirectory() by open for all these ctors and opens() that get a String with the index directory. If we do not want to do this, I suggest to deprecate all these methods and tell the user to open the directory themselfes and pass Directory instances to Writer/Reader. This would help cleaning up the code immense later, because we can remove all these closeDirectory checks/pass-throughs everywhere, which are silly... This makes the code completely not-understandable. In my opinion, one should open/close the directory himself and close it after usage.

          Show
          Uwe Schindler added a comment - - edited I am not so happy , but I work on it. Currently my problem is more a failure for the test in LUCENE-1453 . After reopening a DirectoryIndexReader, sometimes (not always) the directory is closed, even with the 1453-workaround. This failure depends on the random seed in testFSDirectoryReopen2() and is reproducible also with the current version of FSDirectory. It seems that 1453 is not completely fixed. I am looking now since 5 hourcs and cannot find the error, the code consists of a lot of printlns and so on. When I found out, what the problem is, I will perhaps open an additional issue, but it seems, that the problem has to do with the FSDirectory changes. Somewhere the directory is closed, although it should be stay open. But what I can say: The error is simplier to reproduce, if the directory is not cached! I have a lot of other fixes for the failing tests on some platforms, I will post a revised patch some time with getDirectory deprecated and open() again. One thing: For investigating this bug, I changed the IndexReader.open() methods to use a non-cached directory. I think we should do this in the final version, too (also for Indexwriter). So also replace FSDir.getDirectory() by open for all these ctors and opens() that get a String with the index directory. If we do not want to do this, I suggest to deprecate all these methods and tell the user to open the directory themselfes and pass Directory instances to Writer/Reader. This would help cleaning up the code immense later, because we can remove all these closeDirectory checks/pass-throughs everywhere, which are silly... This makes the code completely not-understandable. In my opinion, one should open/close the directory himself and close it after usage.
          Hide
          Michael McCandless added a comment -

          In my opinion, the cached directories vs. instantiated directories have one big advantage:
          They are forced to use the same locking mechanism. So if somebody creates a directory using one LockFactory, writes to the index and in a parallel thread uses another locking mechanism with a separate dir instance, he corrupts his index. So from that point of view, only have one directory instance per resource is a good thing (it does not work from different JVM processes, sure).

          I agree.

          But, I don't think this is a strong enough reason for Lucene to be
          doing such magic under-the-hood, going forward. This magic leads to
          other problems (like LUCENE-1453).

          > Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix.

          This is correct. For unit testing, I found out now, that it is much simplier to check, if all tests would also work with other platforms, if you set the FSDir system property when running the tests. With open() this is currently not possible.

          Excellent!

          Before committing we should confirm all tests pass if we temporarily
          hardwire open to return each of the 3 FSDir impls.

          But I don't think this is reason enough to leave the global system
          property in place for real usage of Lucene.

          Maybe I un-comment-out the caching again, but let getDirectory still use the new behaviour, if the system property is not set. We could then in 3.0 just remove the caching, but let getDirectory() alive. I am not sure.

          But you've still unnecessarily broken back-compat with that. By
          making a new method (open), which does neither the magic singleton
          caching nor the global system prop, back-compat users are guaranteed
          to see no changes.

          In my opinion, this is not really a more serious bw-change than a small behaviour change, that can be written into CHANGES.txt. We have more serious ones.

          I would strongly tend to remove the cache at all and write a warning into CHANGES.txt.

          At all, I do not really think anybody has implemented an own subclass of FSDir. The current patch's bw-change is more, that the protected no-arg ctors no longer exist and are no longer used.

          Why take that chance unnecessarily? What are we gaining by changing
          getDirectory so much in place, vs switching to a new (open) API? It's
          entirely possible apps have subclassed FSDir, rely on the singleton
          cache and rely on the global system prop. Making a new API, and
          deprecating in favor of that, won't affect back-compat users at all.

          Show
          Michael McCandless added a comment - In my opinion, the cached directories vs. instantiated directories have one big advantage: They are forced to use the same locking mechanism. So if somebody creates a directory using one LockFactory, writes to the index and in a parallel thread uses another locking mechanism with a separate dir instance, he corrupts his index. So from that point of view, only have one directory instance per resource is a good thing (it does not work from different JVM processes, sure). I agree. But, I don't think this is a strong enough reason for Lucene to be doing such magic under-the-hood, going forward. This magic leads to other problems (like LUCENE-1453 ). > Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix. This is correct. For unit testing, I found out now, that it is much simplier to check, if all tests would also work with other platforms, if you set the FSDir system property when running the tests. With open() this is currently not possible. Excellent! Before committing we should confirm all tests pass if we temporarily hardwire open to return each of the 3 FSDir impls. But I don't think this is reason enough to leave the global system property in place for real usage of Lucene. Maybe I un-comment-out the caching again, but let getDirectory still use the new behaviour, if the system property is not set. We could then in 3.0 just remove the caching, but let getDirectory() alive. I am not sure. But you've still unnecessarily broken back-compat with that. By making a new method (open), which does neither the magic singleton caching nor the global system prop, back-compat users are guaranteed to see no changes. In my opinion, this is not really a more serious bw-change than a small behaviour change, that can be written into CHANGES.txt. We have more serious ones. I would strongly tend to remove the cache at all and write a warning into CHANGES.txt. At all, I do not really think anybody has implemented an own subclass of FSDir. The current patch's bw-change is more, that the protected no-arg ctors no longer exist and are no longer used. Why take that chance unnecessarily? What are we gaining by changing getDirectory so much in place, vs switching to a new (open) API? It's entirely possible apps have subclassed FSDir, rely on the singleton cache and rely on the global system prop. Making a new API, and deprecating in favor of that, won't affect back-compat users at all.
          Hide
          Uwe Schindler added a comment -

          bq The original purpose of the cache was to ensure each unique directory in the filesystem alway mapped to a single instance of FSDir, so that you could synchronize on that instance and be certain that this is equivalent to synchronizing access to that underlying filesystem directory.

          In my opinion, the cached directories vs. instantiated directories have one big advantage:
          They are forced to use the same locking mechanism. So if somebody creates a directory using one LockFactory, writes to the index and in a parallel thread uses another locking mechanism with a separate dir instance, he corrupts his index. So from that point of view, only have one directory instance per resource is a good thing (it does not work from different JVM processes, sure).

          Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix.

          This is correct. For unit testing, I found out now, that it is much simplier to check, if all tests would also work with other platforms, if you set the FSDir system property when running the tests. With open() this is currently not possible.

          Maybe I un-comment-out the caching again, but let getDirectory still use the new behaviour, if the system property is not set. We could then in 3.0 just remove the caching, but let getDirectory() alive. I am not sure.

          In my opinion, this is not really a more serious bw-change than a small behaviour change, that can be written into CHANGES.txt. We have more serious ones.

          I would strongly tend to remove the cache at all and write a warning into CHANGES.txt.

          At all, I do not really think anybody has implemented an own subclass of FSDir. The current patch's bw-change is more, that the protected no-arg ctors no longer exist and are no longer used.

          Show
          Uwe Schindler added a comment - bq The original purpose of the cache was to ensure each unique directory in the filesystem alway mapped to a single instance of FSDir, so that you could synchronize on that instance and be certain that this is equivalent to synchronizing access to that underlying filesystem directory. In my opinion, the cached directories vs. instantiated directories have one big advantage: They are forced to use the same locking mechanism. So if somebody creates a directory using one LockFactory, writes to the index and in a parallel thread uses another locking mechanism with a separate dir instance, he corrupts his index. So from that point of view, only have one directory instance per resource is a good thing (it does not work from different JVM processes, sure). Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix. This is correct. For unit testing, I found out now, that it is much simplier to check, if all tests would also work with other platforms, if you set the FSDir system property when running the tests. With open() this is currently not possible. Maybe I un-comment-out the caching again, but let getDirectory still use the new behaviour, if the system property is not set. We could then in 3.0 just remove the caching, but let getDirectory() alive. I am not sure. In my opinion, this is not really a more serious bw-change than a small behaviour change, that can be written into CHANGES.txt. We have more serious ones. I would strongly tend to remove the cache at all and write a warning into CHANGES.txt. At all, I do not really think anybody has implemented an own subclass of FSDir. The current patch's bw-change is more, that the protected no-arg ctors no longer exist and are no longer used.
          Hide
          Michael McCandless added a comment -

          I am currently playing with this. For the end-user it is not really needed, that directories are cached. Even if he gets a directory that it is cached, he can see it as a alone one. he can close it (because of refcounting and so on) and do everything, he can do with a single directory, too.

          The original purpose of the cache was to ensure each unique directory in the filesystem alway mapped to a single instance of FSDir, so that you could synchronize on that instance and be certain that this is equivalent to synchronizing access to that underlying filesystem directory.

          Lucene had relied on this at one point, but no longer does. I'm not sure if any apps out there still rely on this, so it's dangerous to simply remove it, especially when we have another option (using a new method "open") that won't break such apps.

          More, the mixing of cached and uncached dirs bring more problems (I am currently investigating).

          We should get to the bottom of this, but these problems are pre-existing to this issue, right? (One could already directly instantiate each directory).

          The move from FSDir to SimpleFSDir is more complicated than it seems. In my opinion, it would be a question, if this move should wait until 3.0.

          As long as we preserve the old getDirectory, back-compatible, this change should have no impact on back-compatibility.

          Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix.

          Show
          Michael McCandless added a comment - I am currently playing with this. For the end-user it is not really needed, that directories are cached. Even if he gets a directory that it is cached, he can see it as a alone one. he can close it (because of refcounting and so on) and do everything, he can do with a single directory, too. The original purpose of the cache was to ensure each unique directory in the filesystem alway mapped to a single instance of FSDir, so that you could synchronize on that instance and be certain that this is equivalent to synchronizing access to that underlying filesystem directory. Lucene had relied on this at one point, but no longer does. I'm not sure if any apps out there still rely on this, so it's dangerous to simply remove it, especially when we have another option (using a new method "open") that won't break such apps. More, the mixing of cached and uncached dirs bring more problems (I am currently investigating). We should get to the bottom of this, but these problems are pre-existing to this issue, right? (One could already directly instantiate each directory). The move from FSDir to SimpleFSDir is more complicated than it seems. In my opinion, it would be a question, if this move should wait until 3.0. As long as we preserve the old getDirectory, back-compatible, this change should have no impact on back-compatibility. Ie, it's only if you use the new FSDir.open() API that you get the new behavior. I intentionally went and fixed tests to use FSDir.open so that we stress the new functionality, which then led us to discover tests making invalid assumptions, which we should then fix.
          Hide
          Uwe Schindler added a comment - - edited

          Caching of FSDirs was completely removed

          I think this must wait until 3.0, ie when we remove all getDirectory
          methods.

          I am currently playing with this. For the end-user it is not really needed, that directories are cached. Even if he gets a directory that it is cached, he can see it as a alone one. he can close it (because of refcounting and so on) and do everything, he can do with a single directory, too.

          So why is it not backwards compatible, when we remove caching? All tests pass here!

          More, the mixing of cached and uncached dirs bring more problems (I am currently investigating).

          I will supply a new patch shortly, with all other bugs fixed (there were more, e.g. with some tests in _TestHelper and so on). The move from FSDir to SimpleFSDir is more complicated than it seems. In my opinion, it would be a question, if this move should wait until 3.0.

          Show
          Uwe Schindler added a comment - - edited Caching of FSDirs was completely removed I think this must wait until 3.0, ie when we remove all getDirectory methods. I am currently playing with this. For the end-user it is not really needed, that directories are cached. Even if he gets a directory that it is cached, he can see it as a alone one. he can close it (because of refcounting and so on) and do everything, he can do with a single directory, too. So why is it not backwards compatible, when we remove caching? All tests pass here! More, the mixing of cached and uncached dirs bring more problems (I am currently investigating). I will supply a new patch shortly, with all other bugs fixed (there were more, e.g. with some tests in _TestHelper and so on). The move from FSDir to SimpleFSDir is more complicated than it seems. In my opinion, it would be a question, if this move should wait until 3.0.
          Hide
          Michael McCandless added a comment -

          Ugh, I'm sorry... I had run the tests only on OS X, Java 1.5, which is
          not 64 bit by default (must specify -d64) – I should have tested the
          different OS's and JREs before committing.

          Thanks for jumping on this, Uwe and Earwin!

          FSDir.open() was removed, the logic was included into FSDir.getDirectory and this un-deprectated: If the system property is missing, the same like in Mikes open() happens: choosing the best impl for platform

          But: the removal of the cache is not back-compatible? (I'm not sure
          how/whether anyone relies on that behavior). And, we are wanting to
          move away from that global System property in choosing the class for
          your FSDir. This is why I switched to open instead of back to
          getDirectory.

          Caching of FSDirs was completely removed

          I think this must wait until 3.0, ie when we remove all getDirectory
          methods.

          FSDir.IndexInput/Output (deprected) was removed (suplicate code) and simply replaced by (deprecated) subclasses of SimpleFSDir ones). This is OK for backwards compatibility.

          Excellent!

          Show
          Michael McCandless added a comment - Ugh, I'm sorry... I had run the tests only on OS X, Java 1.5, which is not 64 bit by default (must specify -d64) – I should have tested the different OS's and JREs before committing. Thanks for jumping on this, Uwe and Earwin! FSDir.open() was removed, the logic was included into FSDir.getDirectory and this un-deprectated: If the system property is missing, the same like in Mikes open() happens: choosing the best impl for platform But: the removal of the cache is not back-compatible? (I'm not sure how/whether anyone relies on that behavior). And, we are wanting to move away from that global System property in choosing the class for your FSDir. This is why I switched to open instead of back to getDirectory. Caching of FSDirs was completely removed I think this must wait until 3.0, ie when we remove all getDirectory methods. FSDir.IndexInput/Output (deprected) was removed (suplicate code) and simply replaced by (deprecated) subclasses of SimpleFSDir ones). This is OK for backwards compatibility. Excellent!
          Hide
          Uwe Schindler added a comment -

          New patch: Now only TestCompoundFile fails, some more imporvements.

          Show
          Uwe Schindler added a comment - New patch: Now only TestCompoundFile fails, some more imporvements.
          Hide
          Uwe Schindler added a comment -

          One comment:
          The part with the reflection and the ctor may be non-backwards compatible, because if somebody overwrites the old FSDir and does not provide the wanted ctor (only the default one), it would fail. The default ctor would not even work, as the init() private internal method (used by the old getDirectory()) is no longer available.
          But this is the same like in the discussion about backwards compatibility. In my opinion, for 2.9, we should make FSDir abstract, too. It is sooooooo seldom, that somebody may create a file based dir impl (nio.2 or what else? Or Earwins MemoryCopiedDir). But if somebody does this, he knows what he does and can fix the compilation errors easily!

          Show
          Uwe Schindler added a comment - One comment: The part with the reflection and the ctor may be non-backwards compatible, because if somebody overwrites the old FSDir and does not provide the wanted ctor (only the default one), it would fail. The default ctor would not even work, as the init() private internal method (used by the old getDirectory()) is no longer available. But this is the same like in the discussion about backwards compatibility. In my opinion, for 2.9, we should make FSDir abstract, too. It is sooooooo seldom, that somebody may create a file based dir impl (nio.2 or what else? Or Earwins MemoryCopiedDir). But if somebody does this, he knows what he does and can fix the compilation errors easily!
          Hide
          Uwe Schindler added a comment - - edited

          This is a revised patch (on current trunk):

          • FSDir.open() was removed, the logic was included into FSDir.getDirectory and this un-deprectated: If the system property is missing, the same like in Mikes open() happens: choosing the best impl for platform
          • Caching of FSDirs was completely removed
          • FSDir.IndexInput/Output (deprected) was removed (suplicate code) and simply replaced by (deprecated) subclasses of SimpleFSDir ones). This is OK for backwards compatibility.
          • protected/package-private ctors were removed, getDirectory uses now reflection to find the (File,LockFactory) ctor

          Some tests currently fail:

          • TestCompoundFile (do not know why)
          • TestLockFactory (test is obsolete, execption can only occur on cached dirs)
          • Sometimes reopen does not work because already closed (see LUCENE-1453)

          TODO:

          • Remove the caching code completely (currently comented out)
          • Fix 1453, because if somebody uses a self-instantiated FSDir and not the default, the reopened dir would use the default again! So fix (maybe clone of FSDir, or LUCENE-1453 is obsolete without caching?).

          This patch also contains an overflow fix in MMapDir for files <2 GB but large. "int*int" should be written as "(long)int*int", if the result maybe large (thanks Earwin!).

          Show
          Uwe Schindler added a comment - - edited This is a revised patch (on current trunk): FSDir.open() was removed, the logic was included into FSDir.getDirectory and this un-deprectated: If the system property is missing, the same like in Mikes open() happens: choosing the best impl for platform Caching of FSDirs was completely removed FSDir.IndexInput/Output (deprected) was removed (suplicate code) and simply replaced by (deprecated) subclasses of SimpleFSDir ones). This is OK for backwards compatibility. protected/package-private ctors were removed, getDirectory uses now reflection to find the (File,LockFactory) ctor Some tests currently fail: TestCompoundFile (do not know why) TestLockFactory (test is obsolete, execption can only occur on cached dirs) Sometimes reopen does not work because already closed (see LUCENE-1453 ) TODO: Remove the caching code completely (currently comented out) Fix 1453, because if somebody uses a self-instantiated FSDir and not the default, the reopened dir would use the default again! So fix (maybe clone of FSDir, or LUCENE-1453 is obsolete without caching?). This patch also contains an overflow fix in MMapDir for files <2 GB but large. "int*int" should be written as "(long)int*int", if the result maybe large (thanks Earwin!).
          Hide
          Uwe Schindler added a comment -

          This issue breaks some platforms (if FSDirectory.open() is used in some tests)!

          Show
          Uwe Schindler added a comment - This issue breaks some platforms (if FSDirectory.open() is used in some tests)!
          Hide
          Uwe Schindler added a comment -

          This issue has some more problems after committing.
          And there are also inconsistencies.
          I am working on a patch, that also un-deprecates FSDir.getDirectory() and removes FSDir.open() again. FSDir.getDirectory() will return the platform-optimal directory, if no system property specifying the directory is set. Also removes some duplicate code in the FSDir.IndexInput/Output by subclassing from SimpleFSDir.IndexInput (but mark deprecated).
          My patch will remove the whole FSDir caching (so getDirectory will always return a new directory) and thus again LUCENE-1453 comes in my mind: LUCENE-1453 currently has the problem, that it only works correct with FSDir.getDirectory() instances, but not with self-instantiated implementations.

          Show
          Uwe Schindler added a comment - This issue has some more problems after committing. And there are also inconsistencies. I am working on a patch, that also un-deprecates FSDir.getDirectory() and removes FSDir.open() again. FSDir.getDirectory() will return the platform-optimal directory, if no system property specifying the directory is set. Also removes some duplicate code in the FSDir.IndexInput/Output by subclassing from SimpleFSDir.IndexInput (but mark deprecated). My patch will remove the whole FSDir caching (so getDirectory will always return a new directory) and thus again LUCENE-1453 comes in my mind: LUCENE-1453 currently has the problem, that it only works correct with FSDir.getDirectory() instances, but not with self-instantiated implementations.
          Hide
          Michael McCandless added a comment -

          Attached patch w/ new defaults. I plan to commit in a day or two.

          Show
          Michael McCandless added a comment - Attached patch w/ new defaults. I plan to commit in a day or two.
          Hide
          Michael McCandless added a comment -

          OK so I think FSDir.open should default to MMapDir on 64 bit machine,
          NIOFSDir on non-windows machines, and SimpleFSDir on windows 32 bit
          machines. I'll rev the patch.

          Next question: does anyone know how to reliably determine if the
          running JRE is 32 or 64 bit? I found this:

          http://forums.java.net/jive/message.jspa?messageID=274406

          but I'm worried about how portable that solution is...

          Show
          Michael McCandless added a comment - OK so I think FSDir.open should default to MMapDir on 64 bit machine, NIOFSDir on non-windows machines, and SimpleFSDir on windows 32 bit machines. I'll rev the patch. Next question: does anyone know how to reliably determine if the running JRE is 32 or 64 bit? I found this: http://forums.java.net/jive/message.jspa?messageID=274406 but I'm worried about how portable that solution is...
          Hide
          Earwin Burrfoot added a comment -

          Wait, are you saying Win 64 bit has problems w/ MMapDir? (I thought you just said Win 32 bit, above).

          I have no lucene experience with Win64, and so I extrapolated from 32bit (as I felt it was more of a Windows issue than anything else). Would be nice if someone actually tries.
          If it works, then the rule sounds like - MMap for all 64bit systems, NIO for 32bit non-win, Simple for 32bit win.

          Is MMapDir fine w/ concurrency?

          It's cool with it if you have enough memory (no frequent paging occurs). I'm not sure about NIOFS vs MMap on memory-constrained systems, if you're competing for disk cache.

          Which way do you think "prime all bytes up front on open" should default?

          This has a side-effect of pushing previous mmaps out of memory if you're memory-constrained. Thus, under certain usage conditions (frequent merging, or something like that + low memory) this feature theoretically could be a disadvantage.
          For me it works well enough to be hardcoded to true

          If anybody's interested, I can also repost an alternative for MMapD - MemoryMirroredD, which wraps any given D and explicitly preloads files in non-chunked byte arrays. It's a bit faster than MMapD and MemoryD (for reading), but has certain disadvantages, like stressing your GC and throwing OOM on merges/optimize if you don't have enough heap (unlike MMapD that silently swaps out).

          Show
          Earwin Burrfoot added a comment - Wait, are you saying Win 64 bit has problems w/ MMapDir? (I thought you just said Win 32 bit, above). I have no lucene experience with Win64, and so I extrapolated from 32bit (as I felt it was more of a Windows issue than anything else). Would be nice if someone actually tries. If it works, then the rule sounds like - MMap for all 64bit systems, NIO for 32bit non-win, Simple for 32bit win. Is MMapDir fine w/ concurrency? It's cool with it if you have enough memory (no frequent paging occurs). I'm not sure about NIOFS vs MMap on memory-constrained systems, if you're competing for disk cache. Which way do you think "prime all bytes up front on open" should default? This has a side-effect of pushing previous mmaps out of memory if you're memory-constrained. Thus, under certain usage conditions (frequent merging, or something like that + low memory) this feature theoretically could be a disadvantage. For me it works well enough to be hardcoded to true If anybody's interested, I can also repost an alternative for MMapD - MemoryMirroredD, which wraps any given D and explicitly preloads files in non-chunked byte arrays. It's a bit faster than MMapD and MemoryD (for reading), but has certain disadvantages, like stressing your GC and throwing OOM on merges/optimize if you don't have enough heap (unlike MMapD that silently swaps out).
          Hide
          Uwe Schindler added a comment -

          From my experiences, MMap is always preferred, if you have 64 bit system. In this case, Java maps the file into address space like a swap file and often used parts are in real memory. So in my opinion, it is always preferable on 64 bit systems. If you have much RAM it is even better (because caching).
          On Linux/Solaris you can see the used address space with TOP. I have a webserver with an about 7 GB big index using nmap. It is reopened very often, so the used address space sometimes goes up to >50 Gigabytes, but this is not a problem, as it is not real memory, its just like a "swap file". The finalizer removes the mapped adress space fairly fast (dependent on usage/closing of old segements).
          The index in this directory is really fast, especially, if you have lots of real RAM, that can be used for buffering. You can even load stored fields for thousands of documents very fast, uninversion is also fast.
          Concurrency is no problem as the mapped file is handled like RAM.

          I always recommend users, who want to use lucene: Use 64 bit systems, -d64 JVM parameter (this enables Java in 64 bit on Solaris), a lot of RAM and MMapDirectory.

          Show
          Uwe Schindler added a comment - From my experiences, MMap is always preferred, if you have 64 bit system. In this case, Java maps the file into address space like a swap file and often used parts are in real memory. So in my opinion, it is always preferable on 64 bit systems. If you have much RAM it is even better (because caching). On Linux/Solaris you can see the used address space with TOP. I have a webserver with an about 7 GB big index using nmap. It is reopened very often, so the used address space sometimes goes up to >50 Gigabytes, but this is not a problem, as it is not real memory, its just like a "swap file". The finalizer removes the mapped adress space fairly fast (dependent on usage/closing of old segements). The index in this directory is really fast, especially, if you have lots of real RAM, that can be used for buffering. You can even load stored fields for thousands of documents very fast, uninversion is also fast. Concurrency is no problem as the mapped file is handled like RAM. I always recommend users, who want to use lucene: Use 64 bit systems, -d64 JVM parameter (this enables Java in 64 bit on Solaris), a lot of RAM and MMapDirectory.
          Hide
          Michael McCandless added a comment -

          Not completely sure why, but MMap failed for me on Win32 some time ago

          I think on 32 bit JRE we shouldn't choose MMap in FSDir.open().

          It's a one-liner. I can make a patch, but while it works better than vanilla MMapD, I'm not yet sure it's the best approach. As for writing - okay, I'll tell you if it turns out to be anything good.

          Which way do you think "prime all bytes up front on open" should default?

          There are a lot of variables. Just as you said, on 32bit systems you have to take care of address space. So, nice for small indexes, bad for big ones. But, mmap in Java cannot be explicitly closed, it is closed in finalizer, so even for a small index if you update really often, you can hit an OOM even though you have enough memory. MMapD failed for me on windows. But it is fast. It is absolutely, totally, uber-indespensible, if you have a 64bit system, fat index, memory to spare and require lots of fast searches.

          So, you can probably enable it for non-Win 64bit?

          Wait, are you saying Win 64 bit has problems w/ MMapDir? (I thought you just said Win 32 bit, above).

          Is MMapDir fine w/ concurrency? (I'd assume so). So, if you had the choice (ie, you're running in env w/ plenty of virtual mem), MMapDir would be preferred over NIOFSDir?

          On a 64 bit env that doesn't have that much RAM, MMapDir should fare no worse than NIOFSDir, right? Ie both are competing for the same IO cache. And so on a 64 bit JRE perhaps the default should always be MMAPDir.

          Show
          Michael McCandless added a comment - Not completely sure why, but MMap failed for me on Win32 some time ago I think on 32 bit JRE we shouldn't choose MMap in FSDir.open(). It's a one-liner. I can make a patch, but while it works better than vanilla MMapD, I'm not yet sure it's the best approach. As for writing - okay, I'll tell you if it turns out to be anything good. Which way do you think "prime all bytes up front on open" should default? There are a lot of variables. Just as you said, on 32bit systems you have to take care of address space. So, nice for small indexes, bad for big ones. But, mmap in Java cannot be explicitly closed, it is closed in finalizer, so even for a small index if you update really often, you can hit an OOM even though you have enough memory. MMapD failed for me on windows. But it is fast. It is absolutely, totally, uber-indespensible, if you have a 64bit system, fat index, memory to spare and require lots of fast searches. So, you can probably enable it for non-Win 64bit? Wait, are you saying Win 64 bit has problems w/ MMapDir? (I thought you just said Win 32 bit, above). Is MMapDir fine w/ concurrency? (I'd assume so). So, if you had the choice (ie, you're running in env w/ plenty of virtual mem), MMapDir would be preferred over NIOFSDir? On a 64 bit env that doesn't have that much RAM, MMapDir should fare no worse than NIOFSDir, right? Ie both are competing for the same IO cache. And so on a 64 bit JRE perhaps the default should always be MMAPDir.
          Hide
          Earwin Burrfoot added a comment -

          Yay for base class + three concrete impls (haven't yet looked at the patch).

          I was tempted to choose MMapDir on 64 bit Windows JRE, but it makes me nervous...

          Not completely sure why, but MMap failed for me on Win32 some time ago

          Is this something you could share? Sounds useful! Likewise if you extend MMapDir for writing...

          It's a one-liner. I can make a patch, but while it works better than vanilla MMapD, I'm not yet sure it's the best approach. As for writing - okay, I'll tell you if it turns out to be anything good.

          Does anyone have a strong sense of when MMapDir is / is not an appropriate choice?

          There are a lot of variables. Just as you said, on 32bit systems you have to take care of address space. So, nice for small indexes, bad for big ones. But, mmap in Java cannot be explicitly closed, it is closed in finalizer, so even for a small index if you update really often, you can hit an OOM even though you have enough memory. MMapD failed for me on windows. But it is fast. It is absolutely, totally, uber-indespensible, if you have a 64bit system, fat index, memory to spare and require lots of fast searches.

          So, you can probably enable it for non-Win 64bit?

          Show
          Earwin Burrfoot added a comment - Yay for base class + three concrete impls (haven't yet looked at the patch). I was tempted to choose MMapDir on 64 bit Windows JRE, but it makes me nervous... Not completely sure why, but MMap failed for me on Win32 some time ago Is this something you could share? Sounds useful! Likewise if you extend MMapDir for writing... It's a one-liner. I can make a patch, but while it works better than vanilla MMapD, I'm not yet sure it's the best approach. As for writing - okay, I'll tell you if it turns out to be anything good. Does anyone have a strong sense of when MMapDir is / is not an appropriate choice? There are a lot of variables. Just as you said, on 32bit systems you have to take care of address space. So, nice for small indexes, bad for big ones. But, mmap in Java cannot be explicitly closed, it is closed in finalizer, so even for a small index if you update really often, you can hit an OOM even though you have enough memory. MMapD failed for me on windows. But it is fast. It is absolutely, totally, uber-indespensible, if you have a 64bit system, fat index, memory to spare and require lots of fast searches. So, you can probably enable it for non-Win 64bit?
          Hide
          Michael McCandless added a comment -

          Attached patch:

          • I created SimpleFSDir (the renaming for FSDir), and left FSDir to
            be the base class (in 3.0 it will become abstract). FSDir
            implements most methods, handles locking, etc., so the subclass
            will just have to implement openInput/createOutput.
          • Added FSDir.open static methods; currently they simply choose
            NIOFSDir on non-Windows OS, and FSDir on Windows OS. I was
            tempted to choose MMapDir on 64 bit Windows JRE, but it makes me
            nervous...
          • Strengthened javadocs
          • Cutover most unit tests to FSDir.open
          Show
          Michael McCandless added a comment - Attached patch: I created SimpleFSDir (the renaming for FSDir), and left FSDir to be the base class (in 3.0 it will become abstract). FSDir implements most methods, handles locking, etc., so the subclass will just have to implement openInput/createOutput. Added FSDir.open static methods; currently they simply choose NIOFSDir on non-Windows OS, and FSDir on Windows OS. I was tempted to choose MMapDir on 64 bit Windows JRE, but it makes me nervous... Strengthened javadocs Cutover most unit tests to FSDir.open
          Hide
          Michael McCandless added a comment -

          In the end, I'm unsure if it's a good idea to fold anything into FSD at all.

          OK, you convinced me: let's keep separate classes.

          But I'd like to split the current FSDir into an abstract FSDir and a
          subclass SimpleFSDir. Then, we have three subclasses of FSDir
          (Simple, NIO, MMap). And strengthen the javadocs so SimpleFSDir's
          concurrency limitations are clear.

          I assume your initial aim is to provide users with best impl for current platform without making them think. What about static factory that chooses between several impls?

          Right, that's my goal: there should be a single obvious way to ask for
          a Directory that uses the file system, and that way should have good
          defaults.

          FSDirectory is unfortunately the obvious way now, but it's usually a
          poor choice.

          For example, my version of MMapD uses MappedByteBuffer.load() on creating MMapIndexInput.

          Is this something you could share? Sounds useful! Likewise if you
          extend MMapDir for writing...

          Does anyone have a strong sense of when MMapDir is / is not an
          appropriate choice? I've seen some users report good performance, eg:

          http://mail-archives.apache.org/mod_mbox/lucene-java-user/200603.mbox/%3C20060313025744.18818.qmail@station174.com%3E

          MMap eats into the virtual memory of the process, so on 32 bit JRE
          that obviously a very real concern.

          As the separate public constructors and deprecation of FSDirectory.open() is new in 2.9

          Right we are free to change things still (hmm: a nice side effect of
          NOT releasing very often; incentives aren't quite right...).

          But the old API was FSDirectory.getDirectory.

          I think we should add a static FSDirectory.create() that returns a
          good default impl given your OS.

          Show
          Michael McCandless added a comment - In the end, I'm unsure if it's a good idea to fold anything into FSD at all. OK, you convinced me: let's keep separate classes. But I'd like to split the current FSDir into an abstract FSDir and a subclass SimpleFSDir. Then, we have three subclasses of FSDir (Simple, NIO, MMap). And strengthen the javadocs so SimpleFSDir's concurrency limitations are clear. I assume your initial aim is to provide users with best impl for current platform without making them think. What about static factory that chooses between several impls? Right, that's my goal: there should be a single obvious way to ask for a Directory that uses the file system, and that way should have good defaults. FSDirectory is unfortunately the obvious way now, but it's usually a poor choice. For example, my version of MMapD uses MappedByteBuffer.load() on creating MMapIndexInput. Is this something you could share? Sounds useful! Likewise if you extend MMapDir for writing... Does anyone have a strong sense of when MMapDir is / is not an appropriate choice? I've seen some users report good performance, eg: http://mail-archives.apache.org/mod_mbox/lucene-java-user/200603.mbox/%3C20060313025744.18818.qmail@station174.com%3E MMap eats into the virtual memory of the process, so on 32 bit JRE that obviously a very real concern. As the separate public constructors and deprecation of FSDirectory.open() is new in 2.9 Right we are free to change things still (hmm: a nice side effect of NOT releasing very often; incentives aren't quite right...). But the old API was FSDirectory.getDirectory. I think we should add a static FSDirectory.create() that returns a good default impl given your OS.
          Hide
          Uwe Schindler added a comment -

          In the end, I'm unsure if it's a good idea to fold anything into FSD at all. Too much different stuff in one class becons for spahetti code I assume your initial aim is to provide users with best impl for current platform without making them think.

          That is my problem with the whole issue, too. For me, the three directory impl are too different, to be merged together. Especially if writing is also affected.

          Using an additional parameter in the ctor specifying the type of directory is almost the same like three classes with much cleaner code. The only thing you do not get is the automatic directory impl choosing for filesystem dirs. But the solution to this are static factories.

          What about static factory that chooses between several impls? We had static factory before, it stinked because you had to chose impl through system property, it pooled directories and we had no public constructors. But now we have public constructors If one needs, he uses constructor directly. If one is lazy and wants us to choose for him he uses old API, that simply changes semantics a bit.

          That would be great. As the separate public constructors and deprecation of FSDirectory.open() is new in 2.9, we can change it. Maybe an automatism behind open() if the System.property is unset would be good. Advanced users may still instantiate the directories using the public ctor.

          Show
          Uwe Schindler added a comment - In the end, I'm unsure if it's a good idea to fold anything into FSD at all. Too much different stuff in one class becons for spahetti code I assume your initial aim is to provide users with best impl for current platform without making them think. That is my problem with the whole issue, too. For me, the three directory impl are too different, to be merged together. Especially if writing is also affected. Using an additional parameter in the ctor specifying the type of directory is almost the same like three classes with much cleaner code. The only thing you do not get is the automatic directory impl choosing for filesystem dirs. But the solution to this are static factories. What about static factory that chooses between several impls? We had static factory before, it stinked because you had to chose impl through system property, it pooled directories and we had no public constructors. But now we have public constructors If one needs, he uses constructor directly. If one is lazy and wants us to choose for him he uses old API, that simply changes semantics a bit. That would be great. As the separate public constructors and deprecation of FSDirectory.open() is new in 2.9, we can change it. Maybe an automatism behind open() if the System.property is unset would be good. Advanced users may still instantiate the directories using the public ctor.
          Hide
          Earwin Burrfoot added a comment -

          Excellent point - I think that makes sense. I'll fold it in as well.

          Doesn't make sense to me. :/ MMapD has different characteristics. It can also have it's own configurable properties, which are irrelevant for NIOFSD and FSD.

          For example, my version of MMapD uses MappedByteBuffer.load() on creating MMapIndexInput. That way the cost of loading stuff into memory is paid upfront on reopening the reader, instead of during first several searches.
          If we want to publish this feature into trunk, we'll end up with additional parameter on MMapD constructor, that governs whether we want to preload mmapped files, or not.
          Now imagine constructor for FSD-that-folds-it-all, which has 'type', and a boolean setting that's relevant only for one of the types.

          I also think of trying to use mmap for writing. If that proves to be beneficial, MMapD won't share much with FSD anymore.

          In the end, I'm unsure if it's a good idea to fold anything into FSD at all. Too much different stuff in one class becons for spahetti code I assume your initial aim is to provide users with best impl for current platform without making them think. What about static factory that chooses between several impls? We had static factory before, it stinked because you had to chose impl through system property, it pooled directories and we had no public constructors. But now we have public constructors If one needs, he uses constructor directly. If one is lazy and wants us to choose for him he uses old API, that simply changes semantics a bit.

          Show
          Earwin Burrfoot added a comment - Excellent point - I think that makes sense. I'll fold it in as well. Doesn't make sense to me. :/ MMapD has different characteristics. It can also have it's own configurable properties, which are irrelevant for NIOFSD and FSD. For example, my version of MMapD uses MappedByteBuffer.load() on creating MMapIndexInput. That way the cost of loading stuff into memory is paid upfront on reopening the reader, instead of during first several searches. If we want to publish this feature into trunk, we'll end up with additional parameter on MMapD constructor, that governs whether we want to preload mmapped files, or not. Now imagine constructor for FSD-that-folds-it-all, which has 'type', and a boolean setting that's relevant only for one of the types. I also think of trying to use mmap for writing. If that proves to be beneficial, MMapD won't share much with FSD anymore. In the end, I'm unsure if it's a good idea to fold anything into FSD at all. Too much different stuff in one class becons for spahetti code I assume your initial aim is to provide users with best impl for current platform without making them think. What about static factory that chooses between several impls? We had static factory before, it stinked because you had to chose impl through system property, it pooled directories and we had no public constructors. But now we have public constructors If one needs, he uses constructor directly. If one is lazy and wants us to choose for him he uses old API, that simply changes semantics a bit.
          Hide
          Michael McCandless added a comment -

          maybe not use a boolean for the mode, instead an enum?

          OK I'll switch to oal.Parameter enum emulator.

          Mike, are you going to absorb MMapDirectory into FSDirectory as well? It fits "when one's index is in the filesystem".

          Excellent point – I think that makes sense. I'll fold it in as well.

          Show
          Michael McCandless added a comment - maybe not use a boolean for the mode, instead an enum? OK I'll switch to oal.Parameter enum emulator. Mike, are you going to absorb MMapDirectory into FSDirectory as well? It fits "when one's index is in the filesystem". Excellent point – I think that makes sense. I'll fold it in as well.
          Hide
          Uwe Schindler added a comment -

          java 1.5, unless you're going to write it by hand

          Lucene has the o.a.l.utils.Parameter class for that, whih is used for e.g. Field.Store.

          Show
          Uwe Schindler added a comment - java 1.5, unless you're going to write it by hand Lucene has the o.a.l.utils.Parameter class for that, whih is used for e.g. Field.Store.
          Hide
          Earwin Burrfoot added a comment -

          enum

          java 1.5, unless you're going to write it by hand

          Mike, are you going to absorb MMapDirectory into FSDirectory as well? It fits "when one's index is in the filesystem".

          Show
          Earwin Burrfoot added a comment - enum java 1.5, unless you're going to write it by hand Mike, are you going to absorb MMapDirectory into FSDirectory as well? It fits "when one's index is in the filesystem".
          Hide
          Uwe Schindler added a comment -

          Hi Mike,
          maybe not use a boolean for the mode, instead an enum?

          Show
          Uwe Schindler added a comment - Hi Mike, maybe not use a boolean for the mode, instead an enum?
          Hide
          Michael McCandless added a comment -

          Attached patch. I plan to commit in a day or two.

          Show
          Michael McCandless added a comment - Attached patch. I plan to commit in a day or two.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Michael McCandless
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development