Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core/store
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The directory is very simple and useful if you have an index that you
      know fully fits into available RAM. You could also use FileSwitchDir if
      you want to leave some files (eg stored fields or term vectors) on disk.

      It wraps any other Directory and delegates all writing (IndexOutput) to
      it, but for reading (IndexInput), it allocates a single byte[] and fully
      reads the file in and then serves requests off that single byte[]. It's
      more GC friendly than RAMDir since it only allocates a single array per
      file.

      It has a few nocommits still, but all tests pass if I wrap the delegate
      inside MockDirectoryWrapper using this.

      I tested with 1M Wikipedia english index (would like to test w/ 10M docs
      but I don't have enough RAM...); it seems to give a nice speedup:

                      Task    QPS base StdDev base  QPS cachedStdDev cached      Pct diff
                   Respell      197.00        7.27      203.19        8.17   -4% -   11%
                  PKLookup      121.12        2.80      125.46        3.20   -1% -    8%
                    Fuzzy2       66.62        2.62       69.91        2.85   -3% -   13%
                    Fuzzy1      206.20        6.47      222.21        6.52    1% -   14%
             TermGroup100K      160.14        6.62      175.71        3.79    3% -   16%
                    Phrase       34.85        0.40       38.75        0.61    8% -   14%
            TermBGroup100K      363.75       15.74      406.98       13.23    3% -   20%
                  SpanNear       53.08        1.11       59.53        2.94    4% -   20%
          TermBGroup100K1P      222.53        9.78      252.86        5.96    6% -   21%
              SloppyPhrase       70.36        2.05       79.95        4.48    4% -   23%
                  Wildcard      238.10        4.29      272.78        4.97   10% -   18%
                 OrHighMed      123.49        4.85      149.32        4.66   12% -   29%
                   Prefix3      288.46        8.10      350.40        5.38   16% -   26%
                OrHighHigh       76.46        3.27       93.13        2.96   13% -   31%
                    IntNRQ       92.25        2.12      113.47        5.74   14% -   32%
                      Term      757.12       39.03      958.62       22.68   17% -   36%
               AndHighHigh      103.03        4.48      133.89        3.76   21% -   39%
                AndHighMed      376.36       16.58      493.99       10.00   23% -   40%
      
      1. LUCENE-4123.patch
        10 kB
        Michael McCandless
      2. LUCENE-4123.patch
        8 kB
        Michael McCandless
      3. LUCENE-4123.patch
        6 kB
        Michael McCandless
      4. LUCENE-4123.patch
        8 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        I am not sure if we really need that directory. With my changes in LUCENE-3659 we can handle that easily (also for files > 2 GiB). LUCENE-3659 makes the buf size of RAMDir configureable (depending on IOContext while writing) and when you do new RAMDirectory(otherDir) - to cache the whole dir in RAM - it will use the maximum possible buffer size for the underlying file (2 GiB) - as we dont write and need no smaller buf size.

        Actually I think the two dirs have different use cases.

        So I think we should do both: 1) fix RAMDir to do better buffering
        (LUCENE-3659) and 2) add this new dir.

        RAMDir is good for pure in-memory indices (for testing, or transient
        usage, etc.) or for pulling in a read-only index from disk, while
        CachingRAMDir (I think we should rename it to CachingDirWrapper) is
        good if you want to write to the index but also want persistence,
        since all writes go straight to the wrapped directory.

        I don't think the limitations of this dir (max 2.1 GB file size) need
        to block committing ... the javadocs call this out, and we can improve
        it later. It could be wrapping the byte[] in ByteBuffer and using
        ByteBufferII doesn't lose any perf: that would be great. But we can
        explore that after committing.

        But definitely +1 to get LUCENE-3659 in...

        Show
        Michael McCandless added a comment - I am not sure if we really need that directory. With my changes in LUCENE-3659 we can handle that easily (also for files > 2 GiB). LUCENE-3659 makes the buf size of RAMDir configureable (depending on IOContext while writing) and when you do new RAMDirectory(otherDir) - to cache the whole dir in RAM - it will use the maximum possible buffer size for the underlying file (2 GiB) - as we dont write and need no smaller buf size. Actually I think the two dirs have different use cases. So I think we should do both: 1) fix RAMDir to do better buffering ( LUCENE-3659 ) and 2) add this new dir. RAMDir is good for pure in-memory indices (for testing, or transient usage, etc.) or for pulling in a read-only index from disk, while CachingRAMDir (I think we should rename it to CachingDirWrapper) is good if you want to write to the index but also want persistence, since all writes go straight to the wrapped directory. I don't think the limitations of this dir (max 2.1 GB file size) need to block committing ... the javadocs call this out, and we can improve it later. It could be wrapping the byte[] in ByteBuffer and using ByteBufferII doesn't lose any perf: that would be great. But we can explore that after committing. But definitely +1 to get LUCENE-3659 in...
        Hide
        Uwe Schindler added a comment -

        Mike,
        I am not sure if we really need that directory. With my changes in LUCENE-3659 we can handle that easily (also for files > 2 GiB). LUCENE-3659 makes the buf size of RAMDir configureable (depending on IOContext while writing) and when you do new RAMDirectory(otherDir) - to cache the whole dir in RAM - it will use the maximum possible buffer size for the underlying file (2 GiB) - as we dont write and need no smaller buf size.

        We should really get LUCENE-3659 in. The only missing parts are:

        • make RAMFile visible to ConcurrentMap after IndexOutput is closed, so we dont need synchronization on RAMFile
        • use maybe Robert's cool ByteBufferIndexInput from LUCENE-4364
        Show
        Uwe Schindler added a comment - Mike, I am not sure if we really need that directory. With my changes in LUCENE-3659 we can handle that easily (also for files > 2 GiB). LUCENE-3659 makes the buf size of RAMDir configureable (depending on IOContext while writing) and when you do new RAMDirectory(otherDir) - to cache the whole dir in RAM - it will use the maximum possible buffer size for the underlying file (2 GiB) - as we dont write and need no smaller buf size. We should really get LUCENE-3659 in. The only missing parts are: make RAMFile visible to ConcurrentMap after IndexOutput is closed, so we dont need synchronization on RAMFile use maybe Robert's cool ByteBufferIndexInput from LUCENE-4364
        Hide
        Michael McCandless added a comment -

        New patch, also overriding createSlicer so that opening CFS files is more efficient. But this resulted in a new nocommit: how to [efficiently] enforce the slice length so you get EOFE if you try to read beyond your slice ...

        Tests pass.

        Show
        Michael McCandless added a comment - New patch, also overriding createSlicer so that opening CFS files is more efficient. But this resulted in a new nocommit: how to [efficiently] enforce the slice length so you get EOFE if you try to read beyond your slice ... Tests pass.
        Hide
        Michael McCandless added a comment -

        Thanks Robert! That's awesome feedback ... new patch.

        I also added a check in SimpleFSIndexInput.clone() to throw ACE if it was closed already.

        Show
        Michael McCandless added a comment - Thanks Robert! That's awesome feedback ... new patch. I also added a check in SimpleFSIndexInput.clone() to throw ACE if it was closed already.
        Hide
        Robert Muir added a comment -

        also readBytes should not catch ArrayIndexOutOfBoundsException. it must be the more general IndexOutOfBoundsException.

        Show
        Robert Muir added a comment - also readBytes should not catch ArrayIndexOutOfBoundsException. it must be the more general IndexOutOfBoundsException.
        Hide
        Robert Muir added a comment -

        looks good... i dont really like that close() is a no-op and that seek() has no checks (since its deferred, if you seek somewhere negative you wont know until later).

        you could probably fix both of these, e.g. keep the byte[] final but let close() turn set the position negative, catch NegativeArray and throw ACE.
        then just throw IAE on seek if the incoming long is negative at least, since you reserve it to mean closed.

        I also don't like that its a delegator.

        should the underlying read check for BufferedII and pass useBuffer=false?

        Show
        Robert Muir added a comment - looks good... i dont really like that close() is a no-op and that seek() has no checks (since its deferred, if you seek somewhere negative you wont know until later). you could probably fix both of these, e.g. keep the byte[] final but let close() turn set the position negative, catch NegativeArray and throw ACE. then just throw IAE on seek if the incoming long is negative at least, since you reserve it to mean closed. I also don't like that its a delegator. should the underlying read check for BufferedII and pass useBuffer=false?
        Hide
        Michael McCandless added a comment -

        New patch, catching AIOOBE and throwing EOFException, and removing the specialized impls.

        I moved it to core temporarily to make it easier to test (add -Dtests.directory=CachingRAMDirectory). I'll move it back to misc/ before committing ...

        Show
        Michael McCandless added a comment - New patch, catching AIOOBE and throwing EOFException, and removing the specialized impls. I moved it to core temporarily to make it easier to test (add -Dtests.directory=CachingRAMDirectory). I'll move it back to misc/ before committing ...
        Hide
        Michael McCandless added a comment -

        I believe it is safe ... eg all tests pass if I wrap MDW's delegate w/ this in newDirectory ...

        I'll update the patch w/ Uwe and Robert's suggestions ...

        Show
        Michael McCandless added a comment - I believe it is safe ... eg all tests pass if I wrap MDW's delegate w/ this in newDirectory ... I'll update the patch w/ Uwe and Robert's suggestions ...
        Hide
        Shai Erera added a comment -

        Besides Uwe's ideas for improvements, is this Directory operable? I.e., if you chose to commit what you have accomplished so far, do tests fail? Is it safe to use?

        I'm thinking "progress, not perfection" – we can always introduce improvements later.

        Show
        Shai Erera added a comment - Besides Uwe's ideas for improvements, is this Directory operable? I.e., if you chose to commit what you have accomplished so far, do tests fail? Is it safe to use? I'm thinking "progress, not perfection" – we can always introduce improvements later.
        Hide
        Michael McCandless added a comment -

        OK thanks Uwe!

        Show
        Michael McCandless added a comment - OK thanks Uwe!
        Hide
        Uwe Schindler added a comment -

        Are we sure the catch + rethrow adds no cost?

        Yes! It is definitely also less work for hotspot than asserts

        In general, throwing exceptions instead of if statements is used because of this. The exception matrix is in the metadata of a method and just defines the goto statements in the exceptional case. If you dont catch exception, this matrix only contains the bubble-up entry, otherwise the jvm goes to our bytecode that simply rethrows. this rethrowing is seldom, so overhead bywrapping the inner exception is neglectible.

        Exceptions for array indexes are implemented by traps on most processors, so in the exceptional case (AIOOBE) is not happening it does not exist.

        I won't have time for this any time soon so if you want to work on it Uwe feel free!

        I have some time. The RAMDir issue is also open (it should also use index exceptions instead of if statements), so can look into it next week.

        Show
        Uwe Schindler added a comment - Are we sure the catch + rethrow adds no cost? Yes! It is definitely also less work for hotspot than asserts In general, throwing exceptions instead of if statements is used because of this. The exception matrix is in the metadata of a method and just defines the goto statements in the exceptional case. If you dont catch exception, this matrix only contains the bubble-up entry, otherwise the jvm goes to our bytecode that simply rethrows. this rethrowing is seldom, so overhead bywrapping the inner exception is neglectible. Exceptions for array indexes are implemented by traps on most processors, so in the exceptional case (AIOOBE) is not happening it does not exist. I won't have time for this any time soon so if you want to work on it Uwe feel free! I have some time. The RAMDir issue is also open (it should also use index exceptions instead of if statements), so can look into it next week.
        Hide
        Michael McCandless added a comment -

        You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code).

        +1. Are we sure the catch + rethrow adds no cost?

        Though, I think tests don't actually fail as is, because I intentionally skip caching segments_N. Probably we should improve that to skip any file that's opened with readOnce=true.

        Can we make this IndexInput impl extend ByteArrayDataInput somehow?

        +1

        I won't have time for this any time soon so if you want to work on it Uwe feel free!

        Show
        Michael McCandless added a comment - You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code). +1. Are we sure the catch + rethrow adds no cost? Though, I think tests don't actually fail as is, because I intentionally skip caching segments_N. Probably we should improve that to skip any file that's opened with readOnce=true. Can we make this IndexInput impl extend ByteArrayDataInput somehow? +1 I won't have time for this any time soon so if you want to work on it Uwe feel free!
        Hide
        Michael McCandless added a comment -

        You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code).

        +1. Are we sure the catch + rethrow adds no cost?

        Though, I think tests don't actually fail as is, because I intentionally skip caching segments_N. Probably we should improve that to skip any file that's opened with readOnce=true.

        Can we make this IndexInput impl extend ByteArrayDataInput somehow?

        +1

        I won't have time for this any time soon so if you want to work on it Uwe feel free!

        Show
        Michael McCandless added a comment - You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code). +1. Are we sure the catch + rethrow adds no cost? Though, I think tests don't actually fail as is, because I intentionally skip caching segments_N. Probably we should improve that to skip any file that's opened with readOnce=true. Can we make this IndexInput impl extend ByteArrayDataInput somehow? +1 I won't have time for this any time soon so if you want to work on it Uwe feel free!
        Hide
        Uwe Schindler added a comment -

        When thinking more about the patch:
        Can we make this IndexInput impl extend ByteArrayDataInput somehow? I would also like to fix ByteArrayDataInput to correctly rethrow AIOOBE and remove the vint methods. We already did tests with FSTs that showed that the code duplication is not helpful.

        Show
        Uwe Schindler added a comment - When thinking more about the patch: Can we make this IndexInput impl extend ByteArrayDataInput somehow? I would also like to fix ByteArrayDataInput to correctly rethrow AIOOBE and remove the vint methods. We already did tests with FSTs that showed that the code duplication is not helpful.
        Hide
        Uwe Schindler added a comment -

        You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code). This does not have any speed effect. Otherwise some tests will definitely fail.

        Show
        Uwe Schindler added a comment - You should make the II correctly throw IOExceptions like MMap does, so catch the AIOOBE and rethrow as EOFException (just copy the code). This does not have any speed effect. Otherwise some tests will definitely fail.
        Hide
        Michael McCandless added a comment -

        I dont think it buys anything to code dup the readVint/vlong here. it should be compiled to the same code. e.g. mmapdir doesnt do this.

        I think you're right! Here are the results w/ the code dup removed (same static seed as previous 5M doc results):

                        Task    QPS base StdDev base  QPS cachedStdDev cached      Pct diff
                      IntNRQ       16.36        0.86       16.92        0.75   -6% -   14%
              TermBGroup1M1P       91.71        3.03       95.07        3.94   -3% -   11%
                 TermGroup1M       58.14        1.00       60.38        1.53    0% -    8%
                TermBGroup1M      103.11        1.76      108.14        2.63    0% -    9%
                     Prefix3      108.83        0.97      115.05        2.89    2% -    9%
                    Wildcard       67.27        0.72       71.22        1.71    2% -    9%
                     Respell      102.29        7.78      109.08        7.22   -7% -   23%
                      Fuzzy2       42.46        2.95       45.51        3.31   -7% -   23%
                      Fuzzy1       72.46        3.55       77.96        4.51   -3% -   19%
                        Term      247.45       17.73      268.17       12.28   -3% -   22%
                   OrHighMed       22.38        1.19       24.47        1.64   -3% -   23%
                  OrHighHigh       18.01        0.92       19.71        1.20   -2% -   22%
                 AndHighHigh       30.79        0.35       33.80        0.37    7% -   12%
                    PKLookup       84.71        2.40       93.95        2.32    5% -   16%
                    SpanNear       10.54        0.13       12.02        0.13   11% -   16%
                  AndHighMed      119.18        1.05      136.64        1.80   12% -   17%
                SloppyPhrase       15.50        0.15       18.26        0.30   14% -   20%
                      Phrase       20.64        0.12       24.94        0.48   17% -   23%
        

        So I'll remove the code dup.

        Show
        Michael McCandless added a comment - I dont think it buys anything to code dup the readVint/vlong here. it should be compiled to the same code. e.g. mmapdir doesnt do this. I think you're right! Here are the results w/ the code dup removed (same static seed as previous 5M doc results): Task QPS base StdDev base QPS cachedStdDev cached Pct diff IntNRQ 16.36 0.86 16.92 0.75 -6% - 14% TermBGroup1M1P 91.71 3.03 95.07 3.94 -3% - 11% TermGroup1M 58.14 1.00 60.38 1.53 0% - 8% TermBGroup1M 103.11 1.76 108.14 2.63 0% - 9% Prefix3 108.83 0.97 115.05 2.89 2% - 9% Wildcard 67.27 0.72 71.22 1.71 2% - 9% Respell 102.29 7.78 109.08 7.22 -7% - 23% Fuzzy2 42.46 2.95 45.51 3.31 -7% - 23% Fuzzy1 72.46 3.55 77.96 4.51 -3% - 19% Term 247.45 17.73 268.17 12.28 -3% - 22% OrHighMed 22.38 1.19 24.47 1.64 -3% - 23% OrHighHigh 18.01 0.92 19.71 1.20 -2% - 22% AndHighHigh 30.79 0.35 33.80 0.37 7% - 12% PKLookup 84.71 2.40 93.95 2.32 5% - 16% SpanNear 10.54 0.13 12.02 0.13 11% - 16% AndHighMed 119.18 1.05 136.64 1.80 12% - 17% SloppyPhrase 15.50 0.15 18.26 0.30 14% - 20% Phrase 20.64 0.12 24.94 0.48 17% - 23% So I'll remove the code dup.
        Hide
        Michael McCandless added a comment -

        Results for 5M doc index:

                        Task    QPS base StdDev base  QPS cachedStdDev cached      Pct diff
                     Respell      104.06        7.63      108.59        7.55   -9% -   20%
                 TermGroup1M       57.94        1.59       60.70        0.30    1% -    8%
                TermBGroup1M      103.28        2.54      108.51        2.54    0% -   10%
                      Fuzzy2       43.07        2.96       45.32        3.06   -8% -   20%
                      Fuzzy1       72.64        4.73       76.92        4.38   -6% -   19%
              TermBGroup1M1P       90.14        3.03       95.95        3.81   -1% -   14%
                      IntNRQ       16.01        0.95       17.17        0.33    0% -   16%
                    PKLookup       86.21        2.51       92.55        2.59    1% -   13%
                    Wildcard       65.51        3.13       71.00        1.45    1% -   16%
                   OrHighMed       21.64        1.83       23.56        1.24   -4% -   25%
                     Prefix3      105.33        4.94      114.75        2.46    1% -   16%
                  OrHighHigh       17.39        1.45       18.97        0.95   -4% -   24%
                 AndHighHigh       30.05        1.14       33.42        0.88    4% -   18%
                        Term      243.13        9.03      273.92        8.26    5% -   20%
                SloppyPhrase       15.80        0.28       17.84        0.78    6% -   19%
                    SpanNear       10.52        0.14       11.97        0.25    9% -   17%
                  AndHighMed      117.60        3.54      135.91        2.49   10% -   21%
                      Phrase       20.15        0.78       24.22        0.26   14% -   26%
        
        Show
        Michael McCandless added a comment - Results for 5M doc index: Task QPS base StdDev base QPS cachedStdDev cached Pct diff Respell 104.06 7.63 108.59 7.55 -9% - 20% TermGroup1M 57.94 1.59 60.70 0.30 1% - 8% TermBGroup1M 103.28 2.54 108.51 2.54 0% - 10% Fuzzy2 43.07 2.96 45.32 3.06 -8% - 20% Fuzzy1 72.64 4.73 76.92 4.38 -6% - 19% TermBGroup1M1P 90.14 3.03 95.95 3.81 -1% - 14% IntNRQ 16.01 0.95 17.17 0.33 0% - 16% PKLookup 86.21 2.51 92.55 2.59 1% - 13% Wildcard 65.51 3.13 71.00 1.45 1% - 16% OrHighMed 21.64 1.83 23.56 1.24 -4% - 25% Prefix3 105.33 4.94 114.75 2.46 1% - 16% OrHighHigh 17.39 1.45 18.97 0.95 -4% - 24% AndHighHigh 30.05 1.14 33.42 0.88 4% - 18% Term 243.13 9.03 273.92 8.26 5% - 20% SloppyPhrase 15.80 0.28 17.84 0.78 6% - 19% SpanNear 10.52 0.14 11.97 0.25 9% - 17% AndHighMed 117.60 3.54 135.91 2.49 10% - 21% Phrase 20.15 0.78 24.22 0.26 14% - 26%
        Hide
        Robert Muir added a comment -

        I dont think it buys anything to code dup the readVint/vlong here. it should be compiled to the same code. e.g. mmapdir doesnt do this.

        Show
        Robert Muir added a comment - I dont think it buys anything to code dup the readVint/vlong here. it should be compiled to the same code. e.g. mmapdir doesnt do this.
        Hide
        Simon Willnauer added a comment -

        I tested with 1M Wikipedia english index (would like to test w/ 10M docs

        but I don't have enough RAM...); it seems to give a nice speedup:

        #fail!

        Show
        Simon Willnauer added a comment - I tested with 1M Wikipedia english index (would like to test w/ 10M docs but I don't have enough RAM...); it seems to give a nice speedup: #fail!

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development