Lucene - Core
  1. Lucene - Core
  2. LUCENE-2574

Optimize copies between IndexInput and Output

    Details

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

      Description

      We've created an optimized copy of files from Directory to Directory. We've also optimized copyBytes recently. However, we're missing the opposite side of the copy - from IndexInput to Output. I'd like to mimic the FileChannel API by having copyTo on IndexInput and copyFrom on IndexOutput. That way, both sides can optimize the copy process, depending on the type of the IndexInput/Output that they need to copy to/from.

      FSIndexInput/Output can use FileChannel if the two are FS types. RAMInput/OutputStream can copy to/from the buffers directly, w/o going through intermediate ones. Actually, for RAMIn/Out this might be a big win, because it doesn't care about the type of IndexInput/Output given - it just needs to copy to its buffer directly.

      If we do this, I think we can consolidate all Dir.copy() impls down to one (in Directory), and rely on the In/Out ones to do the optimized copy. Plus, it will enable someone to do optimized copies between In/Out outside the scope of Directory.

      If this somehow turns out to be impossible, or won't make sense, then I'd like to optimize RAMDirectory.copy(Dir, src, dest) to not use an intermediate buffer.

      1. LUCENE-2574.patch
        15 kB
        Shai Erera
      2. LUCENE-2574.patch
        4 kB
        Michael McCandless
      3. LUCENE-2574.patch
        43 kB
        Michael McCandless

        Activity

        Hide
        Shai Erera added a comment -

        Patch adds copyBytes to IndexInput and overrides in RAM and FS classes. Also optimizes copyBytes in RAMOutputStream. I'd appreciate if someone can review this. Changes are on 3x

        Show
        Shai Erera added a comment - Patch adds copyBytes to IndexInput and overrides in RAM and FS classes. Also optimizes copyBytes in RAMOutputStream. I'd appreciate if someone can review this. Changes are on 3x
        Hide
        Michael McCandless added a comment -

        Patch looks good Shai!

        It looks like NIOFSIndexInput.copyBytes doesn't have an optimized impl (it's currently the default dir on non-Windows)?

        Show
        Michael McCandless added a comment - Patch looks good Shai! It looks like NIOFSIndexInput.copyBytes doesn't have an optimized impl (it's currently the default dir on non-Windows)?
        Hide
        Shai Erera added a comment -

        I think it borrows the impl from SimpleFSIndexInput, which it overrides? Their impl is the same - both use FileChannel.

        Show
        Shai Erera added a comment - I think it borrows the impl from SimpleFSIndexInput, which it overrides? Their impl is the same - both use FileChannel.
        Hide
        Michael McCandless added a comment -

        Ahhh you're right, OK, excellent!

        Show
        Michael McCandless added a comment - Ahhh you're right, OK, excellent!
        Hide
        Shai Erera added a comment -

        Committed revision 980654 (3x).
        Committed revision 980656 (trunk).

        Show
        Shai Erera added a comment - Committed revision 980654 (3x). Committed revision 980656 (trunk).
        Hide
        Michael McCandless added a comment -

        This optimization causes index corruption, to at least stored fields.

        I've been iterating w/ Karl Wright (see dev thread subject "busywait hang using extracting update handler on trunk"), and tracked it down to a missing seek call in the FSIndexInput.copyBytes method.

        Show
        Michael McCandless added a comment - This optimization causes index corruption, to at least stored fields. I've been iterating w/ Karl Wright (see dev thread subject "busywait hang using extracting update handler on trunk"), and tracked it down to a missing seek call in the FSIndexInput.copyBytes method.
        Hide
        Michael McCandless added a comment -

        Adds a seek to the input FileChannel.

        The randomized testcase in TestIW fails (w/o the fix) if you run it enough times.

        Show
        Michael McCandless added a comment - Adds a seek to the input FileChannel. The randomized testcase in TestIW fails (w/o the fix) if you run it enough times.
        Hide
        Shai Erera added a comment -

        Good catch Mike !

        Calling getChannel() on the first time positions the FileChannel properly - it is the latter times that this seek is needed for. I hope this didn't cause you too much headache .

        Show
        Shai Erera added a comment - Good catch Mike ! Calling getChannel() on the first time positions the FileChannel properly - it is the latter times that this seek is needed for. I hope this didn't cause you too much headache .
        Hide
        Michael McCandless added a comment -

        It was pretty fast to track down once I had a randomized test case showing it...

        Show
        Michael McCandless added a comment - It was pretty fast to track down once I had a randomized test case showing it...
        Hide
        Michael McCandless added a comment -

        Attached patch that shows that we still have a bug, here.

        The patch adds deletions to the randomized test (and disables all other tests in TIW), and sets a seed that fails quickly.

        I'm not sure where the bug is yet...

        Show
        Michael McCandless added a comment - Attached patch that shows that we still have a bug, here. The patch adds deletions to the randomized test (and disables all other tests in TIW), and sets a seed that fails quickly. I'm not sure where the bug is yet...
        Hide
        Shai Erera added a comment -

        Weird .. I've found out that if I disable this call:

        // flush any bytes in the input's buffer.
        numBytes -= fsInput.flushBuffer(this, numBytes);

        from FSIndexOutput.copyBytes, then the test passes.

        But I'm not yet sure why ... I've disabled everything else (IndexInput.copyBytes). It's as if this call copies bytes that should not have been copied. I've added this change because I thought there is a bug in the current copyBytes impl: it uses FileChannel to do the optimized copy, but since SimpleFSIndexInput extends BufferedIndexInput, there might be bytes read to its buffer, and not written yet ...

        I've made the following changes:
        FSIndexOutput.copyBytes:

              SimpleFSIndexInput fsInput = (SimpleFSIndexInput) input;
        
        // change start
        //      // flush any bytes in the input's buffer.
        //      numBytes -= fsInput.flushBuffer(this, numBytes);
              
              // flush any bytes in our buffer
              flush();
        // change end
        
              // do the optimized copy
              FileChannel in = fsInput.file.getChannel();
        

        and rewrote BufferedIndexInput.flushBuffer:

            int toCopy = bufferLength - bufferPosition;
            if (toCopy < numBytes) {
              // We're copying the entire content of the buffer, so update accordingly.
              out.writeBytes(buffer, bufferPosition, toCopy);
              bufferPosition = bufferLength = 0;
              bufferStart += toCopy;
            } else {
              toCopy = (int) numBytes;
              // We are asked to copy less bytes than are available in the buffer.
              out.writeBytes(buffer, bufferPosition, toCopy);
              bufferPosition += toCopy;
            }
            return toCopy;
        

        The test now fails on some other exception (not the assert in addRawDocuments). If I remove the call to flushBuffer, it passes. I still need to understand this.

        Show
        Shai Erera added a comment - Weird .. I've found out that if I disable this call: // flush any bytes in the input's buffer. numBytes -= fsInput.flushBuffer(this, numBytes); from FSIndexOutput.copyBytes, then the test passes. But I'm not yet sure why ... I've disabled everything else (IndexInput.copyBytes). It's as if this call copies bytes that should not have been copied. I've added this change because I thought there is a bug in the current copyBytes impl: it uses FileChannel to do the optimized copy, but since SimpleFSIndexInput extends BufferedIndexInput, there might be bytes read to its buffer, and not written yet ... I've made the following changes: FSIndexOutput.copyBytes: SimpleFSIndexInput fsInput = (SimpleFSIndexInput) input; // change start // // flush any bytes in the input's buffer. // numBytes -= fsInput.flushBuffer( this , numBytes); // flush any bytes in our buffer flush(); // change end // do the optimized copy FileChannel in = fsInput.file.getChannel(); and rewrote BufferedIndexInput.flushBuffer: int toCopy = bufferLength - bufferPosition; if (toCopy < numBytes) { // We're copying the entire content of the buffer, so update accordingly. out.writeBytes(buffer, bufferPosition, toCopy); bufferPosition = bufferLength = 0; bufferStart += toCopy; } else { toCopy = ( int ) numBytes; // We are asked to copy less bytes than are available in the buffer. out.writeBytes(buffer, bufferPosition, toCopy); bufferPosition += toCopy; } return toCopy; The test now fails on some other exception (not the assert in addRawDocuments). If I remove the call to flushBuffer, it passes. I still need to understand this.
        Hide
        Shai Erera added a comment -

        Found the problem – the order of the flushes in FSIndexOutput.copyBytes was wrong. flush() first emptied FSIndexOutput's buffer, but flushBuffer() filled it back. I reversed the calls and the test passes.

        Show
        Shai Erera added a comment - Found the problem – the order of the flushes in FSIndexOutput.copyBytes was wrong. flush() first emptied FSIndexOutput's buffer, but flushBuffer() filled it back. I reversed the calls and the test passes.
        Hide
        Michael McCandless added a comment -

        OK thanks Shai! I'll stress test that, and I'll also add the seek in CSIndexInput.copyBytes.

        Show
        Michael McCandless added a comment - OK thanks Shai! I'll stress test that, and I'll also add the seek in CSIndexInput.copyBytes.
        Hide
        Shai Erera added a comment -

        Mike, did you add the seek to CSIndexInput? I don't see it in the commit log.

        Show
        Shai Erera added a comment - Mike, did you add the seek to CSIndexInput? I don't see it in the commit log.
        Hide
        Shai Erera added a comment -

        Seems like it was left out. I'd like to add this fix, could you please review:

        Index: lucene/src/java/org/apache/lucene/index/CompoundFileReader.java
        ===================================================================
        --- lucene/src/java/org/apache/lucene/index/CompoundFileReader.java	(revision 982137)
        +++ lucene/src/java/org/apache/lucene/index/CompoundFileReader.java	(working copy)
        @@ -310,6 +310,11 @@
                   // If there are more bytes left to copy, delegate the copy task to the
                   // base IndexInput, in case it can do an optimized copy.
                   if (numBytes > 0) {
        +            long start = getFilePointer();
        +            if (start + numBytes > length) {
        +              throw new IOException("read past EOF");
        +            }
        +            base.seek(fileOffset + start);
                     base.copyBytes(out, numBytes);
                   }
                 }
        
        Show
        Shai Erera added a comment - Seems like it was left out. I'd like to add this fix, could you please review: Index: lucene/src/java/org/apache/lucene/index/CompoundFileReader.java =================================================================== --- lucene/src/java/org/apache/lucene/index/CompoundFileReader.java (revision 982137) +++ lucene/src/java/org/apache/lucene/index/CompoundFileReader.java (working copy) @@ -310,6 +310,11 @@ // If there are more bytes left to copy, delegate the copy task to the // base IndexInput, in case it can do an optimized copy. if (numBytes > 0) { + long start = getFilePointer(); + if (start + numBytes > length) { + throw new IOException( "read past EOF" ); + } + base.seek(fileOffset + start); base.copyBytes(out, numBytes); } }
        Hide
        Michael McCandless added a comment -

        Woops I did miss this.

        Shouldn't the if statement be fileOffset + numBytes > length instead?

        Show
        Michael McCandless added a comment - Woops I did miss this. Shouldn't the if statement be fileOffset + numBytes > length instead?
        Hide
        Shai Erera added a comment -

        I've copied it from readInternal:

                  long start = getFilePointer();
                  if(start + len > length)
                    throw new IOException("read past EOF");
                  base.seek(fileOffset + start);
        

        I think it's ok. fileOffset is where this file starts and length is its length. You want to know if you have enough bytes from 'start', no?

        Show
        Shai Erera added a comment - I've copied it from readInternal: long start = getFilePointer(); if (start + len > length) throw new IOException( "read past EOF" ); base.seek(fileOffset + start); I think it's ok. fileOffset is where this file starts and length is its length. You want to know if you have enough bytes from 'start', no?
        Hide
        Michael McCandless added a comment -

        Duh sorry you are correct! fileOffset is the "base" of this file within the CFS.

        Better to finish drinking coffee in the morning before thinking too much...

        Show
        Michael McCandless added a comment - Duh sorry you are correct! fileOffset is the "base" of this file within the CFS. Better to finish drinking coffee in the morning before thinking too much...
        Hide
        Shai Erera added a comment -

        Better to finish drinking coffee in the morning before thinking too much...

        "Better to finish drinking coffee in the morning" ... period !

        Ok so I'll commit this. Thanks !

        Show
        Shai Erera added a comment - Better to finish drinking coffee in the morning before thinking too much... "Better to finish drinking coffee in the morning" ... period ! Ok so I'll commit this. Thanks !
        Hide
        Shai Erera added a comment -

        Committed to 3x and trunk. Hopefully the last time .

        Show
        Shai Erera added a comment - Committed to 3x and trunk. Hopefully the last time .
        Hide
        Grant Ingersoll added a comment -

        Bulk close for 3.1

        Show
        Grant Ingersoll added a comment - Bulk close for 3.1
        Hide
        Matthias Seidel added a comment -

        Uhm, what happened to these optimizations to FSIndexOutput regarding the direct copying from FSIndexInput via its FileChannel? Can't find a trace of it anywhere in the 3.1 release or in the trunk. :/

        Show
        Matthias Seidel added a comment - Uhm, what happened to these optimizations to FSIndexOutput regarding the direct copying from FSIndexInput via its FileChannel? Can't find a trace of it anywhere in the 3.1 release or in the trunk. :/
        Hide
        Robert Muir added a comment -

        I removed them after they caused index corruption, twice.

        Show
        Robert Muir added a comment - I removed them after they caused index corruption, twice.
        Hide
        Matthias Seidel added a comment -

        Oh, sorry to here that. Was looking forward to this optimization. Any chance of it coming back anytime soon?

        Show
        Matthias Seidel added a comment - Oh, sorry to here that. Was looking forward to this optimization. Any chance of it coming back anytime soon?
        Hide
        Robert Muir added a comment -

        See LUCENE-2637 for more discussion.

        Personally I'm not sure this optimization is worth the trouble, I haven't
        seen any benchmarks showing it speeds things up, and its dangerous as hell.

        Show
        Robert Muir added a comment - See LUCENE-2637 for more discussion. Personally I'm not sure this optimization is worth the trouble, I haven't seen any benchmarks showing it speeds things up, and its dangerous as hell.
        Hide
        Matthias Seidel added a comment -

        Ok, didn't know that. Thanks for the quick reply anyway.

        Show
        Matthias Seidel added a comment - Ok, didn't know that. Thanks for the quick reply anyway.

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development