Lucene - Core
  1. Lucene - Core
  2. LUCENE-2283

Possible Memory Leak in StoredFieldsWriter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.9.3, 3.0.2, 3.1, 4.0-ALPHA
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      StoredFieldsWriter creates a pool of PerDoc instances

      this pool will grow but never be reclaimed by any mechanism

      furthermore, each PerDoc instance contains a RAMFile.
      this RAMFile will also never be truncated (and will only ever grow) (as far as i can tell)

      When feeding documents with large number of stored fields (or one large dominating stored field) this can result in memory being consumed in the RAMFile but never reclaimed. Eventually, each pooled PerDoc could grow very large, even if large documents are rare.

      Seems like there should be some attempt to reclaim memory from the PerDoc[] instance pool (or otherwise limit the size of RAMFiles that are cached) etc

      1. LUCENE-2283.patch
        12 kB
        Tim Smith
      2. LUCENE-2283.patch
        13 kB
        Tim Smith
      3. LUCENE-2283.patch
        13 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Merged fix for this back to 29x, 30x. It was already on 3x since we cut that branch after this landed.

        Show
        Michael McCandless added a comment - Merged fix for this back to 29x, 30x. It was already on 3x since we cut that branch after this landed.
        Hide
        Michael McCandless added a comment -

        I'll merge back to 2.9.x / 3.0.x / 3.1.x.

        Show
        Michael McCandless added a comment - I'll merge back to 2.9.x / 3.0.x / 3.1.x.
        Hide
        Shay Banon added a comment -

        Is there a chance that this can also be applied to 3.0.2 / 3.1? It would be really helpful to get this as soon as possible in the next Lucene version.

        Show
        Shay Banon added a comment - Is there a chance that this can also be applied to 3.0.2 / 3.1? It would be really helpful to get this as soon as possible in the next Lucene version.
        Hide
        Michael McCandless added a comment -

        Thanks Tim!

        Show
        Michael McCandless added a comment - Thanks Tim!
        Hide
        Tim Smith added a comment -

        i haven't been able to fully replicate this issue in a unit test scenario,

        however it will definitely resolve that 40M of ram that was allocated and never released for the RAMFiles on the StoredFieldsWriter (keeping that bound to the configured memory size)

        Show
        Tim Smith added a comment - i haven't been able to fully replicate this issue in a unit test scenario, however it will definitely resolve that 40M of ram that was allocated and never released for the RAMFiles on the StoredFieldsWriter (keeping that bound to the configured memory size)
        Hide
        Michael McCandless added a comment -

        New rev attached with some small improvements:

        • Changed RAMOutputStream.reset to set currentBuffer to null (frees up that 1 buffer), and call that instead of close.
        • Removed the separate PerDoc.recycle – now I just do the recycle in the existing PerDoc.reset method

        Also, this patch changes the RAM accounting to count buffers allocated not bytes written to the file. This is a good change, and I think may explain the too-much-memory-allocated problem you saw. Ie if you write tiny docs, Lucene was thinking they consumed tiny memory (not the 1024 bytes that the 1 buffer uses) and thus was undercounting. When mixed in with massive docs this can cause way too much RAM to be allocated.

        Have you been able to test if this patch resolves Lucene's part of the mem growth you were seeing?

        Show
        Michael McCandless added a comment - New rev attached with some small improvements: Changed RAMOutputStream.reset to set currentBuffer to null (frees up that 1 buffer), and call that instead of close. Removed the separate PerDoc.recycle – now I just do the recycle in the existing PerDoc.reset method Also, this patch changes the RAM accounting to count buffers allocated not bytes written to the file. This is a good change, and I think may explain the too-much-memory-allocated problem you saw. Ie if you write tiny docs, Lucene was thinking they consumed tiny memory (not the 1024 bytes that the 1 buffer uses) and thus was undercounting. When mixed in with massive docs this can cause way too much RAM to be allocated. Have you been able to test if this patch resolves Lucene's part of the mem growth you were seeing?
        Hide
        Michael McCandless added a comment -

        Looks great Tim, thanks! I think it's ready to commit... I'll wait a day or two.

        Show
        Michael McCandless added a comment - Looks great Tim, thanks! I think it's ready to commit... I'll wait a day or two.
        Hide
        Tim Smith added a comment -

        Here's a new patch with your suggestions

        Show
        Tim Smith added a comment - Here's a new patch with your suggestions
        Hide
        Tim Smith added a comment -

        I'll work up another patch

        might take me a few minutes to get my head wrapped around the TermVectorsTermsWriter stuff

        Show
        Tim Smith added a comment - I'll work up another patch might take me a few minutes to get my head wrapped around the TermVectorsTermsWriter stuff
        Hide
        Michael McCandless added a comment -

        Patch looks great – thanks Tim!

        A few fixes:

        • I think you should pass false to allocator.getByteBlock in PerDocBuffer.newBuffer. We don't want this allocator to track allocations because this storage used is transient (reset per doc). We only pass true for things like the terms hash, that keeps allocating RAM until it's time to flush.
        • Can you also make the same changes to TermVectorsTermsWriter? I think the same hazard applies to it. It should just use the same pool.
        • Can you add a CHANGES entry?

        Thanks!

        Show
        Michael McCandless added a comment - Patch looks great – thanks Tim! A few fixes: I think you should pass false to allocator.getByteBlock in PerDocBuffer.newBuffer. We don't want this allocator to track allocations because this storage used is transient (reset per doc). We only pass true for things like the terms hash, that keeps allocating RAM until it's time to flush. Can you also make the same changes to TermVectorsTermsWriter? I think the same hazard applies to it. It should just use the same pool. Can you add a CHANGES entry? Thanks!
        Hide
        Tim Smith added a comment -

        Here's a patch for using a pool for stored fields buffers

        Show
        Tim Smith added a comment - Here's a patch for using a pool for stored fields buffers
        Hide
        Tim Smith added a comment -

        i'm working up a patch for the shared byteblock pool for stored field buffers (found a few cycles)

        Show
        Tim Smith added a comment - i'm working up a patch for the shared byteblock pool for stored field buffers (found a few cycles)
        Hide
        Tim Smith added a comment -

        another note is that this was on 64 bit vm

        i've noticed that all the memsize calculations assume 4 byte pointers, so perhaps that can lead to more memory being used that would otherwise be expected (although 256 MB is still well over the 2X mem use that would potentially be expected in that case)

        Show
        Tim Smith added a comment - another note is that this was on 64 bit vm i've noticed that all the memsize calculations assume 4 byte pointers, so perhaps that can lead to more memory being used that would otherwise be expected (although 256 MB is still well over the 2X mem use that would potentially be expected in that case)
        Hide
        Michael McCandless added a comment -

        Yeah it would be good to make the pool shared...

        It still bugs me that yourkit is claiming DW was using 256 MB when you've got a 64 MB ram buffer.... that's spooky.

        Show
        Michael McCandless added a comment - Yeah it would be good to make the pool shared... It still bugs me that yourkit is claiming DW was using 256 MB when you've got a 64 MB ram buffer.... that's spooky.
        Hide
        Tim Smith added a comment -

        latest profile dump has pointed out a non-lucene issue as causing some memory growth

        so feel free to drop down priority

        however it seems like using the bytepool for the stored fields would be good overall

        Show
        Tim Smith added a comment - latest profile dump has pointed out a non-lucene issue as causing some memory growth so feel free to drop down priority however it seems like using the bytepool for the stored fields would be good overall
        Hide
        Tim Smith added a comment -

        I agree. I'll mull over how to do it... unless you're planning on consing up a patch

        I'd love to, but don't have the free cycles at the moment

        How many threads do you pass through IW?

        honestly don't 100% know about the origin of the threads i'm given
        In general, they should be from a static pool, but may be dynamically allocated if the static pool runs out

        One thought i had recently was to control this more tightly by having a limited number of static threads that called IndexWriter methods in case that was the issue (but that would be a pretty big change)

        Show
        Tim Smith added a comment - I agree. I'll mull over how to do it... unless you're planning on consing up a patch I'd love to, but don't have the free cycles at the moment How many threads do you pass through IW? honestly don't 100% know about the origin of the threads i'm given In general, they should be from a static pool, but may be dynamically allocated if the static pool runs out One thought i had recently was to control this more tightly by having a limited number of static threads that called IndexWriter methods in case that was the issue (but that would be a pretty big change)
        Hide
        Michael McCandless added a comment -

        ramBufferSizeMB is 64MB

        Here's the yourkit breakdown per class:

        Hmm – spooky. With ram buffer @ 64MB, DocumentsWriter is using 256MB!? Something is clearly amiss. 40 MB used by StoredFieldsWriter's PerDoc still leaves 152 MB unaccounted for... hmm.

        If i recall correctly, I think the exception was caused by an out of disk space situation (which would recover)

        Oh OK. Though... closing the iW vs calling IW.commit should be not different in that regard. Both should have the same transient disk space usage. It's odd you'd see out of disk for .close but not also for .commit.

        Seems like this would be the best approach as it makes the memory bounded by the configuration of the engine, giving better reuse of byte blocks and better ability to reclaim memory (in DocumentsWriter.balanceRAM())

        I agree. I'll mull over how to do it... unless you're planning on consing up a patch

        How many threads do you pass through IW? Are the threads forever from a static pool, or do they come and go? I'd like to try to simulate your usage (huge docs & tiny docs) in my dev area to see if I can provoke the same behavior.

        Show
        Michael McCandless added a comment - ramBufferSizeMB is 64MB Here's the yourkit breakdown per class: Hmm – spooky. With ram buffer @ 64MB, DocumentsWriter is using 256MB!? Something is clearly amiss. 40 MB used by StoredFieldsWriter's PerDoc still leaves 152 MB unaccounted for... hmm. If i recall correctly, I think the exception was caused by an out of disk space situation (which would recover) Oh OK. Though... closing the iW vs calling IW.commit should be not different in that regard. Both should have the same transient disk space usage. It's odd you'd see out of disk for .close but not also for .commit. Seems like this would be the best approach as it makes the memory bounded by the configuration of the engine, giving better reuse of byte blocks and better ability to reclaim memory (in DocumentsWriter.balanceRAM()) I agree. I'll mull over how to do it... unless you're planning on consing up a patch How many threads do you pass through IW? Are the threads forever from a static pool, or do they come and go? I'd like to try to simulate your usage (huge docs & tiny docs) in my dev area to see if I can provoke the same behavior.
        Hide
        Tim Smith added a comment -

        ramBufferSizeMB is 64MB

        Here's the yourkit breakdown per class:

        • DocumentsWriter - 256 MB
          • TermsHash - 38.7 MB
          • StoredFieldsWriter - 37.5 MB
          • DocumentsWriterThreadState - 36.2 MB
          • DocumentsWriterThreadState - 34.6 MB
          • DocumentsWriterThreadState - 33.8 MB
          • DocumentsWriterThreadState - 27.5 MB
          • DocumentsWriterThreadState - 13.4 MB

        I'm starting to dig into the ThreadStates now to see if anything stands out here

        Hmm, that makes me nervous, because I think in this case the use should be bounded.

        I should be getting a new profile dump at "crash" time soon, so hopefully that will make things clearer

        That doesn't sound good! Can you post some details on this (eg an exception)?

        If i recall correctly, I think the exception was caused by an out of disk space situation (which would recover)
        obviously not much that can be done about this other than adding more disk space, however the situation would recover, but docs would be lost in the interum

        But, anyway, keeping the same IW open and just calling commit is (should be) fine.

        Yeah, this should be the way to go, especially as it results in the pooled buffers not needing to be reallocated/reclaimed/etc, however right now this is the only change i can currently think of that could result in memory issues.

        Yes, that's a great solution - a single pool. But that's a somewhat bigger change.

        Seems like this would be the best approach as it makes the memory bounded by the configuration of the engine, giving better reuse of byte blocks and better ability to reclaim memory (in DocumentsWriter.balanceRAM())

        Show
        Tim Smith added a comment - ramBufferSizeMB is 64MB Here's the yourkit breakdown per class: DocumentsWriter - 256 MB TermsHash - 38.7 MB StoredFieldsWriter - 37.5 MB DocumentsWriterThreadState - 36.2 MB DocumentsWriterThreadState - 34.6 MB DocumentsWriterThreadState - 33.8 MB DocumentsWriterThreadState - 27.5 MB DocumentsWriterThreadState - 13.4 MB I'm starting to dig into the ThreadStates now to see if anything stands out here Hmm, that makes me nervous, because I think in this case the use should be bounded. I should be getting a new profile dump at "crash" time soon, so hopefully that will make things clearer That doesn't sound good! Can you post some details on this (eg an exception)? If i recall correctly, I think the exception was caused by an out of disk space situation (which would recover) obviously not much that can be done about this other than adding more disk space, however the situation would recover, but docs would be lost in the interum But, anyway, keeping the same IW open and just calling commit is (should be) fine. Yeah, this should be the way to go, especially as it results in the pooled buffers not needing to be reallocated/reclaimed/etc, however right now this is the only change i can currently think of that could result in memory issues. Yes, that's a great solution - a single pool. But that's a somewhat bigger change. Seems like this would be the best approach as it makes the memory bounded by the configuration of the engine, giving better reuse of byte blocks and better ability to reclaim memory (in DocumentsWriter.balanceRAM())
        Hide
        Michael McCandless added a comment -

        a yourkit snapshot showed that the PerDocs for an IndexWriter were using ~40M of memory

        What was IW's ramBufferSizeMB when you saw this?

        however i have reports that eventually the memory is completely exhausted resulting in out of memory errors.

        Hmm, that makes me nervous, because I think in this case the use should be bounded.

        As a side note, closing the index writer at commit time would sometimes fail, resulting in some following updates to fail because the index writer was locked and couldn't be reopened until the old index writer was garbage collected, so i don't want to go back to this for commits.

        That doesn't sound good! Can you post some details on this (eg an exception)?

        But, anyway, keeping the same IW open and just calling commit is (should be) fine.

        As far as a fix goes, wouldn't it be better to have the RAMFile's used for stored fields pull and return byte buffers from the byte block pool on the DocumentsWriter?

        Yes, that's a great solution – a single pool. But that's a somewhat bigger change. I guess we can pass a byte[] allocator to RAMFile. It'd have to be a new pool, too (DW's byte blocks are 32KB not the 1KB that RAMFile uses).

        Show
        Michael McCandless added a comment - a yourkit snapshot showed that the PerDocs for an IndexWriter were using ~40M of memory What was IW's ramBufferSizeMB when you saw this? however i have reports that eventually the memory is completely exhausted resulting in out of memory errors. Hmm, that makes me nervous, because I think in this case the use should be bounded. As a side note, closing the index writer at commit time would sometimes fail, resulting in some following updates to fail because the index writer was locked and couldn't be reopened until the old index writer was garbage collected, so i don't want to go back to this for commits. That doesn't sound good! Can you post some details on this (eg an exception)? But, anyway, keeping the same IW open and just calling commit is (should be) fine. As far as a fix goes, wouldn't it be better to have the RAMFile's used for stored fields pull and return byte buffers from the byte block pool on the DocumentsWriter? Yes, that's a great solution – a single pool. But that's a somewhat bigger change. I guess we can pass a byte[] allocator to RAMFile. It'd have to be a new pool, too (DW's byte blocks are 32KB not the 1KB that RAMFile uses).
        Hide
        Tim Smith added a comment -

        I came across this issue looking for a reported memory leak during indexing

        a yourkit snapshot showed that the PerDocs for an IndexWriter were using ~40M of memory (at which point i came across this potentially unbounded memory use in StoredFieldsWriter)
        this snapshot seems more or less at a stable point (memory grows but then returns to a "normal" state), however i have reports that eventually the memory is completely exhausted resulting in out of memory errors.

        I so far have not found any other major culprit in the lucene indexing code.

        This index receives a routine mix of very large and very small documents (which would explain this situation)
        The VM and system have more than ample amount of memory given the buffer size and what should be normal indexing RAM requirements.

        Also, a major difference between this leak not occurring and it showing up is that previously, the IndexWriter was closed when performing commits, now the IndexWriter remains open (just calling IndexWriter.commit()). So, if any memory is leaking during indexing, it is no longer being reclaimed during commit. As a side note, closing the index writer at commit time would sometimes fail, resulting in some following updates to fail because the index writer was locked and couldn't be reopened until the old index writer was garbage collected, so i don't want to go back to this for commits.

        Its possible there is a leak somewhere else (i currently do not have a snapshot right before out of memory issues occur, so currently the only thing that stands out is the PerDoc memory use)

        As far as a fix goes, wouldn't it be better to have the RAMFile's used for stored fields pull and return byte buffers from the byte block pool on the DocumentsWriter? This would allow the memory to be reclaimed based on the index writers buffer size (otherwise there is no configurable way to tune this memory use)

        Show
        Tim Smith added a comment - I came across this issue looking for a reported memory leak during indexing a yourkit snapshot showed that the PerDocs for an IndexWriter were using ~40M of memory (at which point i came across this potentially unbounded memory use in StoredFieldsWriter) this snapshot seems more or less at a stable point (memory grows but then returns to a "normal" state), however i have reports that eventually the memory is completely exhausted resulting in out of memory errors. I so far have not found any other major culprit in the lucene indexing code. This index receives a routine mix of very large and very small documents (which would explain this situation) The VM and system have more than ample amount of memory given the buffer size and what should be normal indexing RAM requirements. Also, a major difference between this leak not occurring and it showing up is that previously, the IndexWriter was closed when performing commits, now the IndexWriter remains open (just calling IndexWriter.commit()). So, if any memory is leaking during indexing, it is no longer being reclaimed during commit. As a side note, closing the index writer at commit time would sometimes fail, resulting in some following updates to fail because the index writer was locked and couldn't be reopened until the old index writer was garbage collected, so i don't want to go back to this for commits. Its possible there is a leak somewhere else (i currently do not have a snapshot right before out of memory issues occur, so currently the only thing that stands out is the PerDoc memory use) As far as a fix goes, wouldn't it be better to have the RAMFile's used for stored fields pull and return byte buffers from the byte block pool on the DocumentsWriter? This would allow the memory to be reclaimed based on the index writers buffer size (otherwise there is no configurable way to tune this memory use)
        Hide
        Michael McCandless added a comment -

        TermVectorsTermsWriter has the same issue.

        You're right: with "irregular" sized documents coming through, you can
        end up with PerDoc instances that waste space, because the RAMFile has
        buffers allocated from past huge docs that the latest tiny docs don't
        use.

        Note that the number of outstanding PerDoc instances is a function of
        how "out of order" the docs are being indexed, because the PerDoc
        holds any state only until that doc can be written to the store files
        (stored fields, term vectors). It's transient.

        EG with a single thread, there will only be one PerDoc – it's written
        immediately. With 2 threads, if you have a massive doc (which thread
        1 get stuck indexing) and then zillions of tiny docs (which thread 2
        burns through, while thread 1 is busy), then you can get a large
        number of PerDocs created, waiting for their turn because thread 1
        hasn't finished yet.

        But this process won't use unbounded RAM – the RAM used by the
        RAMFiles is accounted for, and once it gets too high (10% of the RAM
        buffer size), we forcefully idle the incoming threads until the "out
        of orderness" is resolved. EG in this case, thread 2 will stall until
        thread 1 has finished its doc. That byte accounting does account for
        the allocated but not used byte[1024] inside RAMFile (we use
        RAMFile.sizeInBytes()).

        So... this is not really a memory leak. But it is a potential
        starvation issue, in that if your PerDoc instances all grow to large
        RAMFiles over time (as each has had to service a very large document),
        then it can mean the amount of concurrency that DW allows will become
        "pinched". Especially if these docs are large relative to your
        ram buffer size.

        Are you hitting this issue? Ie seeing poor concurrency during
        indexing despite using many threads, because DW is forcefully idleing
        the threads? It should only happen if you sometimes index docs
        that are larger than RAMBufferSize/10/numberOrIndexingThreads.

        I'll work out a fix. I think we should fix RAMFile.reset to trim its
        buffers using ArrayUtil.getShrinkSize.

        Show
        Michael McCandless added a comment - TermVectorsTermsWriter has the same issue. You're right: with "irregular" sized documents coming through, you can end up with PerDoc instances that waste space, because the RAMFile has buffers allocated from past huge docs that the latest tiny docs don't use. Note that the number of outstanding PerDoc instances is a function of how "out of order" the docs are being indexed, because the PerDoc holds any state only until that doc can be written to the store files (stored fields, term vectors). It's transient. EG with a single thread, there will only be one PerDoc – it's written immediately. With 2 threads, if you have a massive doc (which thread 1 get stuck indexing) and then zillions of tiny docs (which thread 2 burns through, while thread 1 is busy), then you can get a large number of PerDocs created, waiting for their turn because thread 1 hasn't finished yet. But this process won't use unbounded RAM – the RAM used by the RAMFiles is accounted for, and once it gets too high (10% of the RAM buffer size), we forcefully idle the incoming threads until the "out of orderness" is resolved. EG in this case, thread 2 will stall until thread 1 has finished its doc. That byte accounting does account for the allocated but not used byte [1024] inside RAMFile (we use RAMFile.sizeInBytes()). So... this is not really a memory leak. But it is a potential starvation issue, in that if your PerDoc instances all grow to large RAMFiles over time (as each has had to service a very large document), then it can mean the amount of concurrency that DW allows will become "pinched". Especially if these docs are large relative to your ram buffer size. Are you hitting this issue? Ie seeing poor concurrency during indexing despite using many threads, because DW is forcefully idleing the threads? It should only happen if you sometimes index docs that are larger than RAMBufferSize/10/numberOrIndexingThreads. I'll work out a fix. I think we should fix RAMFile.reset to trim its buffers using ArrayUtil.getShrinkSize.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Tim Smith
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development