Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-1186

[PATCH] Clear ThreadLocal instances in close()

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.3.1, 2.4
    • Fix Version/s: 2.4.1, 2.9
    • Component/s: None
    • Labels:
      None
    • Environment:

      any

    • Lucene Fields:
      New, Patch Available

      Description

      As already found out in LUCENE-436, there seems to be a garbage collection problem with ThreadLocals at certain constellations, resulting in an OutOfMemoryError.
      The resolution there was to remove the reference to the ThreadLocal value when calling the close() method of the affected classes (see FieldsReader and TermInfosReader).
      For Java < 5.0, this can effectively be done by calling threadLocal.set(null); for Java >= 5.0, we would call threadLocal.remove()

      Analogously, this should be done in any class which creates ThreadLocal values

      Right now, two classes of the core API make use of ThreadLocals, but do not properly remove their references to the ThreadLocal value
      1. org.apache.lucene.index.SegmentReader
      2. org.apache.lucene.analysis.Analyzer

      For SegmentReader, I have attached a simple patch.
      For Analyzer, there currently is no patch because Analyzer does not provide a close() method (future to-do?)

      1. LUCENE-1186.patch
        3 kB
        Michael McCandless
      2. LUCENE-1186.patch
        3 kB
        Michael McCandless
      3. LUCENE-1186-SegmentReader.patch
        0.3 kB
        Christian Kohlschütter

        Activity

        Hide
        ck@newsclub.de Christian Kohlschütter added a comment -

        Patch: Adds termVectorsLocal.set(null) to SegmentReader#close()

        Show
        ck@newsclub.de Christian Kohlschütter added a comment - Patch: Adds termVectorsLocal.set(null) to SegmentReader#close()
        Hide
        otis Otis Gospodnetic added a comment -

        Is there really a memory leak issue here? I don't recall all the details of LUCENE-436, but I think the problem was not with ThreadLocal... Is there a way for you to demonstrate this memory issue?

        Show
        otis Otis Gospodnetic added a comment - Is there really a memory leak issue here? I don't recall all the details of LUCENE-436 , but I think the problem was not with ThreadLocal... Is there a way for you to demonstrate this memory issue?
        Hide
        ck@newsclub.de Christian Kohlschütter added a comment -

        This issue is rather a prophylactic one – until now, I have not encountered an OutOfMemoryError or slowdown etc.

        However, I think it is a good practice to release all resources as soon as an object is not used anymore. For SegmentReader, this is the case when #close() is called. More, as noted in LUCENE-436, some VMs (also recent ones) indeed seem to have problems when ThreadLocal values are not released, so I think it is not just a cosmetic issue.

        Show
        ck@newsclub.de Christian Kohlschütter added a comment - This issue is rather a prophylactic one – until now, I have not encountered an OutOfMemoryError or slowdown etc. However, I think it is a good practice to release all resources as soon as an object is not used anymore. For SegmentReader, this is the case when #close() is called. More, as noted in LUCENE-436 , some VMs (also recent ones) indeed seem to have problems when ThreadLocal values are not released, so I think it is not just a cosmetic issue.
        Hide
        rviper Robert Starzer added a comment - - edited

        i'm using quartz schedules to trigger indexing tasks

        since the analyzer class is using a thread local -> should i reuse an analyzer (e.g singleton pattern) in this case (quartz job reusing threads (=tread pool) -> thread local data never gets freed)...)?

        Mem dump fragment (after out of memory):

        Class name Shallow Heap Retained HeapPercentage
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8070 QuartzScheduler_Worker-3 Thread 120 55.914.144 21,65%

        • java.lang.ThreadLocal$ThreadLocalMap @ 0xdf32a20 24 55.913.760 21,65%
        '- java.lang.ThreadLocal$ThreadLocalMap$Entry[16384] @ 0x163a61b8 65.552 55.913.736 21,65%
        java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe090ca8 32 9.608 0,00%
        java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe0f6b88 ....
        java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe5d3df8 ....
        org.apache.lucene.analysis.StopAnalyzer$SavedStreams @ 0xe090cc8 24 9.560 0,00%
        org.apache.lucene.analysis.LowerCaseTokenizer @ 0xe090ce0 32 8.520 0,00%
        org.apache.lucene.analysis.CharArraySet @ 0xe090e30 24 968 0,00%
        org.apache.lucene.analysis.StopFilter @ 0xe090e18 24 24 0,00%
        org.apache.lucene.analysis.StopAnalyzer @ 0xe0911f8 24 24 0,00%
        java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe629d80 ....
        java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xeb53510 ....
        ......
        ......

        quartz overview (eclipse memory analysis):

        Class name Shallow Heap | Retained Heap | Percentage
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8070 QuartzScheduler_Worker-3 Thread| 120 | 55.914.144 | 21,65%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7a50 QuartzScheduler_Worker-7 Thread| 120 | 30.684.056 | 11,88%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7710 QuartzScheduler_Worker-9 Thread| 120 | 19.464.024 | 7,54%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7ee8 QuartzScheduler_Worker-4 Thread| 120 | 14.813.640 | 5,74%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e78c8 QuartzScheduler_Worker-8 Thread| 120 | 11.154.576 | 4,32%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e81f8 QuartzScheduler_Worker-2 Thread| 120 | 8.403.544 | 3,25%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8380 QuartzScheduler_Worker-1 Thread| 120 | 8.334.552 | 3,23%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7d60 QuartzScheduler_Worker-5 Thread| 120 | 8.314.904 | 3,22%
        org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8508 QuartzScheduler_Worker-0 Thread| 120 | 8.267.376 | 3,20%

        Show
        rviper Robert Starzer added a comment - - edited i'm using quartz schedules to trigger indexing tasks since the analyzer class is using a thread local -> should i reuse an analyzer (e.g singleton pattern) in this case (quartz job reusing threads (=tread pool) -> thread local data never gets freed)...)? Mem dump fragment (after out of memory): Class name Shallow Heap Retained HeapPercentage org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8070 QuartzScheduler_Worker-3 Thread 120 55.914.144 21,65% java.lang.ThreadLocal$ThreadLocalMap @ 0xdf32a20 24 55.913.760 21,65% '- java.lang.ThreadLocal$ThreadLocalMap$Entry [16384] @ 0x163a61b8 65.552 55.913.736 21,65% java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe090ca8 32 9.608 0,00% java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe0f6b88 .... java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe5d3df8 .... org.apache.lucene.analysis.StopAnalyzer$SavedStreams @ 0xe090cc8 24 9.560 0,00% org.apache.lucene.analysis.LowerCaseTokenizer @ 0xe090ce0 32 8.520 0,00% org.apache.lucene.analysis.CharArraySet @ 0xe090e30 24 968 0,00% org.apache.lucene.analysis.StopFilter @ 0xe090e18 24 24 0,00% org.apache.lucene.analysis.StopAnalyzer @ 0xe0911f8 24 24 0,00% java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xe629d80 .... java.lang.ThreadLocal$ThreadLocalMap$Entry @ 0xeb53510 .... ...... ...... quartz overview (eclipse memory analysis): Class name Shallow Heap | Retained Heap | Percentage org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8070 QuartzScheduler_Worker-3 Thread| 120 | 55.914.144 | 21,65% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7a50 QuartzScheduler_Worker-7 Thread| 120 | 30.684.056 | 11,88% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7710 QuartzScheduler_Worker-9 Thread| 120 | 19.464.024 | 7,54% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7ee8 QuartzScheduler_Worker-4 Thread| 120 | 14.813.640 | 5,74% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e78c8 QuartzScheduler_Worker-8 Thread| 120 | 11.154.576 | 4,32% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e81f8 QuartzScheduler_Worker-2 Thread| 120 | 8.403.544 | 3,25% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8380 QuartzScheduler_Worker-1 Thread| 120 | 8.334.552 | 3,23% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e7d60 QuartzScheduler_Worker-5 Thread| 120 | 8.314.904 | 3,22% org.quartz.simpl.SimpleThreadPool$WorkerThread @ 0xb4e8508 QuartzScheduler_Worker-0 Thread| 120 | 8.267.376 | 3,20%
        Hide
        mikemccand Michael McCandless added a comment -

        Re-using a single analyzer should work around this...

        As of LUCENE-1383, we now have a CloseableThreadLocal, and we've used
        it instead of ThreadLocal in many places inside Lucene.

        However, Analyzer still uses ThreadLocal, because it doesn't have a
        close() method. I think we should simply add a close() method to
        Analyzer, which then closes the ThreadLocal (attached patch).

        I plan to commit in a day or two, and back port to 2.4.1.

        Show
        mikemccand Michael McCandless added a comment - Re-using a single analyzer should work around this... As of LUCENE-1383 , we now have a CloseableThreadLocal, and we've used it instead of ThreadLocal in many places inside Lucene. However, Analyzer still uses ThreadLocal, because it doesn't have a close() method. I think we should simply add a close() method to Analyzer, which then closes the ThreadLocal (attached patch). I plan to commit in a day or two, and back port to 2.4.1.
        Hide
        mikemccand Michael McCandless added a comment -

        New patch, giving credit to Christian. Thanks Christian!

        Show
        mikemccand Michael McCandless added a comment - New patch, giving credit to Christian. Thanks Christian!
        Hide
        rviper Robert Starzer added a comment -

        great! thanks!
        IMHO, some kind of IOC container for these lifecycle/caching challenges would make it more transparent - or setting some strategy for different environments

        Show
        rviper Robert Starzer added a comment - great! thanks! IMHO, some kind of IOC container for these lifecycle/caching challenges would make it more transparent - or setting some strategy for different environments
        Hide
        mikemccand Michael McCandless added a comment -

        IMHO, some kind of IOC container for these lifecycle/caching challenges would make it more transparent - or setting some strategy for different environments

        This sounds interesting – can you flesh out some concrete details so we can discuss it? EG are you picturing a single opaque class instance that'd hold all thread-local like state that Lucene needs to store, which could then be directly controlled by the app (ie, close()'d when the app is done for now interacting with Lucene)?

        Show
        mikemccand Michael McCandless added a comment - IMHO, some kind of IOC container for these lifecycle/caching challenges would make it more transparent - or setting some strategy for different environments This sounds interesting – can you flesh out some concrete details so we can discuss it? EG are you picturing a single opaque class instance that'd hold all thread-local like state that Lucene needs to store, which could then be directly controlled by the app (ie, close()'d when the app is done for now interacting with Lucene)?
        Hide
        rviper Robert Starzer added a comment -

        you could use e.g. spring and specific spring-bean-scopes [spring isn't the only choice ;-) ]
        (e.g. request/session scope for web apps, thread local scope (via org.springframework.aop.target.ThreadLocalTargetSource) for none-web apps), or even prototyp scope if nothing
        should be reused

        the main problem however is that i do not know why ThreadLocal objects are used in lucene; i somewhere read about synchronization issues, but IMHO only the developer/user knows
        if synchronization is really necessary;

        could you please explain to me why/when ThreadLocals are used?

        Show
        rviper Robert Starzer added a comment - you could use e.g. spring and specific spring-bean-scopes [spring isn't the only choice ;-) ] (e.g. request/session scope for web apps, thread local scope (via org.springframework.aop.target.ThreadLocalTargetSource) for none-web apps), or even prototyp scope if nothing should be reused the main problem however is that i do not know why ThreadLocal objects are used in lucene; i somewhere read about synchronization issues, but IMHO only the developer/user knows if synchronization is really necessary; could you please explain to me why/when ThreadLocals are used?
        Hide
        mikemccand Michael McCandless added a comment -

        could you please explain to me why/when ThreadLocals are used?

        Lucene largely uses them to avoid fine-grained synchronization.

        EG when you call IndexReader.document(int) we use ThreadLocal to store
        a "private" clone for that thread. Likewise for term vectors, for
        caching terms info, and for analyzers to reuse TokenStream instances.

        Show
        mikemccand Michael McCandless added a comment - could you please explain to me why/when ThreadLocals are used? Lucene largely uses them to avoid fine-grained synchronization. EG when you call IndexReader.document(int) we use ThreadLocal to store a "private" clone for that thread. Likewise for term vectors, for caching terms info, and for analyzers to reuse TokenStream instances.
        Hide
        rviper Robert Starzer added a comment -

        ok thanks!

        https://issues.apache.org/jira/browse/LUCENE-1383
        "If closing all index readers and writers releases all Lucene thread locals it's great."

        -> is this true (only for SegmentReader)?

        another interesting thing for me: is there a reason for the ThreadLocals not being defined as static?

        Show
        rviper Robert Starzer added a comment - ok thanks! https://issues.apache.org/jira/browse/LUCENE-1383 "If closing all index readers and writers releases all Lucene thread locals it's great." -> is this true (only for SegmentReader)? another interesting thing for me: is there a reason for the ThreadLocals not being defined as static?
        Hide
        mikemccand Michael McCandless added a comment -

        -> is this true (only for SegmentReader)?

        It's true, for all readers (Multi*Readers close the underlying SegmentReaders, which then close their ThreadLocals).

        is there a reason for the ThreadLocals not being defined as static?

        Well... if we made them static then we'd have to further key (internally) on the SegmentReader, and then on closing the SegmentReader we'd have to go and try to remove those entries somehow.

        Show
        mikemccand Michael McCandless added a comment - -> is this true (only for SegmentReader)? It's true, for all readers (Multi*Readers close the underlying SegmentReaders, which then close their ThreadLocals). is there a reason for the ThreadLocals not being defined as static? Well... if we made them static then we'd have to further key (internally) on the SegmentReader, and then on closing the SegmentReader we'd have to go and try to remove those entries somehow.
        Hide
        rviper Robert Starzer added a comment -

        "EG are you picturing a single opaque class instance that'd hold all thread-local"

        I think this would solve the problem. The real impl should be configureable via e.g. properties file (keep it simple) to further implement/change to a no-thread-local-at-all-policy if lucene is being used i an thread-pool environment (quartz, web requests,...), or an impl using spring beans, ... (two or three different strategies...)

        Show
        rviper Robert Starzer added a comment - "EG are you picturing a single opaque class instance that'd hold all thread-local" I think this would solve the problem. The real impl should be configureable via e.g. properties file (keep it simple) to further implement/change to a no-thread-local-at-all-policy if lucene is being used i an thread-pool environment (quartz, web requests,...), or an impl using spring beans, ... (two or three different strategies...)

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            ck@newsclub.de Christian Kohlschütter
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development