Lucene - Core
  1. Lucene - Core
  2. LUCENE-3200

Cleanup MMapDirectory to use only one MMapIndexInput impl with mapping sized of powers of 2

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Robert and me discussed a little bit after Mike's investigations, that using SingleMMapIndexinput together with MultiMMapIndexInput leads to hotspot slowdowns sometimes.

      We had the following ideas:

      • MultiMMapIndexInput is almost as fast as SingleMMapIndexInput, as the switching between buffer boundaries is done in exception catch blocks. So normal code path is always the same like for Single*
      • Only the seek method uses strange calculations (the modulo is totally bogus, it could be simply: int bufOffset = (int) (pos % maxBufSize); - very strange way of calculating modulo in the original code)
      • Because of speed we suggest to no longer use arbitrary buffer sizes. We should pass only the power of 2 to the indexinput as size. All calculations in seek and anywhere else would be simple bit shifts and AND operations (the and masks for the modulo can be calculated in the ctor like NumericUtils does when calculating precisionSteps).
      • the maximum buffer size will now be 2^30, not 2^31-1. But thats not an issue at all. In my opinion, a buffer size of 2^31-1 is stupid in all cases, as it will no longer fit page boundaries and mmapping gets harder for the O/S.

      We will provide a patch with those cleanups.

      1. LUCENE-3200.patch
        10 kB
        Uwe Schindler
      2. LUCENE-3200.patch
        10 kB
        Uwe Schindler
      3. LUCENE-3200.patch
        14 kB
        Uwe Schindler
      4. LUCENE-3200.patch
        15 kB
        Robert Muir
      5. LUCENE-3200.patch
        16 kB
        Uwe Schindler
      6. LUCENE-3200_tests.patch
        4 kB
        Robert Muir

        Activity

        Hide
        Uwe Schindler added a comment -

        MMapDirectory missed a explicit nulling of curBuf, commited trunk revision: 1205152, 3x in revision: 1205153

        Show
        Uwe Schindler added a comment - MMapDirectory missed a explicit nulling of curBuf, commited trunk revision: 1205152, 3x in revision: 1205153
        Hide
        Robert Muir added a comment -

        bulk close for 3.3

        Show
        Robert Muir added a comment - bulk close for 3.3
        Hide
        Uwe Schindler added a comment -

        Thanks to Robert for help debugging my stupid + vs | problem and lots of fruitful discussions about the whole stuff and how to improve Thanks to Mike for testing on beast!

        Now you can refactor CFSIndexInput & Co!

        Show
        Uwe Schindler added a comment - Thanks to Robert for help debugging my stupid + vs | problem and lots of fruitful discussions about the whole stuff and how to improve Thanks to Mike for testing on beast! Now you can refactor CFSIndexInput & Co!
        Hide
        Uwe Schindler added a comment -

        Committed trunk revision: 1135537
        Committed 3.x revision: 1135538

        Show
        Uwe Schindler added a comment - Committed trunk revision: 1135537 Committed 3.x revision: 1135538
        Hide
        Simon Willnauer added a comment -

        +1 this looks awesome. Gute Arbeit Uwe

        Show
        Simon Willnauer added a comment - +1 this looks awesome. Gute Arbeit Uwe
        Hide
        Robert Muir added a comment -

        +1, great work Uwe.

        Show
        Robert Muir added a comment - +1, great work Uwe.
        Hide
        Michael McCandless added a comment -

        +1 to commit!

        In my stress NRT test (runs optimize on a full Wiki index with ongoing
        indexing / reopening), without this patch, I see performance drop
        substantially (like 180 QPS down to 140 QPS) when the JVM cuts over to
        the optimized segment. With the patch I see it jump up a bit after
        the optimize completes! So this seems to make hotspot's job
        easier...

        Show
        Michael McCandless added a comment - +1 to commit! In my stress NRT test (runs optimize on a full Wiki index with ongoing indexing / reopening), without this patch, I see performance drop substantially (like 180 QPS down to 140 QPS) when the JVM cuts over to the optimized segment. With the patch I see it jump up a bit after the optimize completes! So this seems to make hotspot's job easier...
        Hide
        Uwe Schindler added a comment -

        Little cleanups & improvements:

        • made readByte() consistent with readBytes() [catch block using remaining-while loop for size=0 buffers]
        • renamed field names and variables to use chunkSize consistently

        I think it's ready to commit, we should only wait for Mike to check on beast.

        Show
        Uwe Schindler added a comment - Little cleanups & improvements: made readByte() consistent with readBytes() [catch block using remaining-while loop for size=0 buffers] renamed field names and variables to use chunkSize consistently I think it's ready to commit, we should only wait for Mike to check on beast.
        Hide
        Robert Muir added a comment -

        same as uwe's patch, but i also nuked the previous hack in TestTermVectorsReader, as MMapDir returns read past EOF now like the others.

        Show
        Robert Muir added a comment - same as uwe's patch, but i also nuked the previous hack in TestTermVectorsReader, as MMapDir returns read past EOF now like the others.
        Hide
        Uwe Schindler added a comment -

        Same patch with Robert's tests included.

        Show
        Uwe Schindler added a comment - Same patch with Robert's tests included.
        Hide
        Uwe Schindler added a comment -

        New patch with some minor issues fixed:

        • fixed the RuntimeException
        • fixed readByte to throw EOF if we are at the end of the n-1 th buffer. as buffer n may be size 0, we will throw BufferUnderFlow in the chatch block. I added hasRemaining() there, so its consistent with readBytes.
        • The check for an invalid power was bogus (0 is allowed, leads to buffer size 1)
        • The check for RandomAccessFile too big for maximum buffer size did not respect the additional buffer. nrBuffers can then overflow easily
        Show
        Uwe Schindler added a comment - New patch with some minor issues fixed: fixed the RuntimeException fixed readByte to throw EOF if we are at the end of the n-1 th buffer. as buffer n may be size 0, we will throw BufferUnderFlow in the chatch block. I added hasRemaining() there, so its consistent with readBytes. The check for an invalid power was bogus (0 is allowed, leads to buffer size 1) The check for RandomAccessFile too big for maximum buffer size did not respect the additional buffer. nrBuffers can then overflow easily
        Hide
        Robert Muir added a comment -

        here are some additional stress tests for mmap

        Show
        Robert Muir added a comment - here are some additional stress tests for mmap
        Hide
        Robert Muir added a comment -

        at a glance the patch is looking really good overall! I'll help with some review and testing.

        Show
        Robert Muir added a comment - at a glance the patch is looking really good overall! I'll help with some review and testing.
        Hide
        Uwe Schindler added a comment -

        Here the patch.

        Show
        Uwe Schindler added a comment - Here the patch.
        Hide
        Robert Muir added a comment -

        also, we can fix the issue Shai brought up for the 3.1 VOTE while we are here.

        in seek(long pos) i think we should do:

        try {
         ...
         position()
         ...
        } catch (IllegalArgumentException e) {
          if (pos < 0) 
            throw exc;
          else 
            throw new IOException("read past EOF"); 
        }
        

        This would be more consistent with NIOFS/SimpleFS from an exception perspective.

        Show
        Robert Muir added a comment - also, we can fix the issue Shai brought up for the 3.1 VOTE while we are here. in seek(long pos) i think we should do: try { ... position() ... } catch (IllegalArgumentException e) { if (pos < 0) throw exc; else throw new IOException( "read past EOF" ); } This would be more consistent with NIOFS/SimpleFS from an exception perspective.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development