Lucene - Core
  1. Lucene - Core
  2. LUCENE-1472

DateTools.stringToDate() can cause lock contention under load

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 3.1, 4.0-ALPHA
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Load testing our application (the JIRA Issue Tracker) has shown that threads spend a lot of time blocked in DateTools.stringToDate().

      The stringToDate() method uses a singleton SimpleDateFormat object to parse the dates.
      Each call to SimpleDateFormat.parse() is synchronized because SimpleDateFormat is not thread safe.

      1. LUCENE-1472.patch
        8 kB
        Uwe Schindler
      2. LUCENE-1472.patch
        12 kB
        Uwe Schindler

        Activity

        Hide
        Grant Ingersoll added a comment -

        Bulk close for 3.1

        Show
        Grant Ingersoll added a comment - Bulk close for 3.1
        Hide
        Uwe Schindler added a comment -

        Committed trunk revision: 1063493
        Committed 3.x revision: 1063494

        Show
        Uwe Schindler added a comment - Committed trunk revision: 1063493 Committed 3.x revision: 1063494
        Hide
        Uwe Schindler added a comment -

        Better patch with also the stupid enum declared as enum. Now the constant lookup should even faster (in table-switch statement).

        Will commit soon.

        Show
        Uwe Schindler added a comment - Better patch with also the stupid enum declared as enum. Now the constant lookup should even faster (in table-switch statement). Will commit soon.
        Hide
        Uwe Schindler added a comment -

        Here the patch, using one ThreadLocal statically.

        (theoretically this class still contains a non-enum which shold in reality an enum. We can change this later or deprecate this class at all)

        I will commit tomorrow to trunk and 3.x

        Show
        Uwe Schindler added a comment - Here the patch, using one ThreadLocal statically. (theoretically this class still contains a non-enum which shold in reality an enum. We can change this later or deprecate this class at all) I will commit tomorrow to trunk and 3.x
        Hide
        Uwe Schindler added a comment -

        This is not longer highest priority, but the fix using ThreadLocal without close seems to be the best possibility. We use this in our local applications, too when we have a pool of DateFormats. I will take care on applying this patch.

        In general, this NumericRangeQuery and NumericField should be used when dates should be indexed.

        Show
        Uwe Schindler added a comment - This is not longer highest priority, but the fix using ThreadLocal without close seems to be the best possibility. We use this in our local applications, too when we have a pool of DateFormats. I will take care on applying this patch. In general, this NumericRangeQuery and NumericField should be used when dates should be indexed.
        Hide
        Michael McCandless added a comment -

        Removing 2.9 target.

        Show
        Michael McCandless added a comment - Removing 2.9 target.
        Hide
        Mark Lassau added a comment -

        An update for anyone interested:

        The test run that showed this contention suffered even worse contention in another component of our application.
        Since we have fixed that component, we have no longer observed contention in DateTools.stringToDate().

        For that reason, this issue is not now considered a priority for us.

        Show
        Mark Lassau added a comment - An update for anyone interested: The test run that showed this contention suffered even worse contention in another component of our application. Since we have fixed that component, we have no longer observed contention in DateTools.stringToDate(). For that reason, this issue is not now considered a priority for us.
        Hide
        Michael McCandless added a comment -

        I agree: ThreadLocal, despite its problems, is probably the best solution here. The memory cost ought to be tiny.

        We could also use CloseableThreadLocal (which works around the problems of ThreadLocal), and then allow DateTools to be instantiated, and add a close() method to it (vs the all-static methods we have today). This way the application could close the DateTools instance and reliably free up the tiny amount of memory used.

        Show
        Michael McCandless added a comment - I agree: ThreadLocal, despite its problems, is probably the best solution here. The memory cost ought to be tiny. We could also use CloseableThreadLocal (which works around the problems of ThreadLocal), and then allow DateTools to be instantiated, and add a close() method to it (vs the all-static methods we have today). This way the application could close the DateTools instance and reliably free up the tiny amount of memory used.
        Hide
        robert engels added a comment -

        The last comment was tested using Java 5.

        It is my understanding that in Java 6, synchronization has become even cheaper - although object creation is cheaper as well - although 99% of the instantiation time of SimpleDateFormat is in the init code, not the object creation.

        I know there has been a lot of discussion of the "problems" with ThreadLocals... I've been a part of most of them - but for these very small objects, the typical ThreadLocal memory issues don't really apply.

        Show
        robert engels added a comment - The last comment was tested using Java 5. It is my understanding that in Java 6, synchronization has become even cheaper - although object creation is cheaper as well - although 99% of the instantiation time of SimpleDateFormat is in the init code, not the object creation. I know there has been a lot of discussion of the "problems" with ThreadLocals... I've been a part of most of them - but for these very small objects, the typical ThreadLocal memory issues don't really apply.
        Hide
        robert engels added a comment -

        If you review the source for SimpleDateFormat you will see that it internally performs some synchronization during initialization anyway (uses some Hashtables), and also that the initialization cost is pretty high (lots of code), so a static sync'd copy is probably best.

        Outside of that, the ThreadLocal is the way to go.

        My tests shows that the instantiation time is 2x longer than the typical parse time.

        Show
        robert engels added a comment - If you review the source for SimpleDateFormat you will see that it internally performs some synchronization during initialization anyway (uses some Hashtables), and also that the initialization cost is pretty high (lots of code), so a static sync'd copy is probably best. Outside of that, the ThreadLocal is the way to go. My tests shows that the instantiation time is 2x longer than the typical parse time.
        Hide
        Mark Lassau added a comment -

        My original thought was ThreadLocal, but I tend to try and avoid these where possible.

        I wanted to test the speed on creating a new Calendar (rather than SimpleDateFormat) for each request, but was worried about the instantiation overhead.
        Daniel's comment above reinforces this concern to some extent, so we'll have to wait and compare benchmarks.

        Show
        Mark Lassau added a comment - My original thought was ThreadLocal, but I tend to try and avoid these where possible. I wanted to test the speed on creating a new Calendar (rather than SimpleDateFormat) for each request, but was worried about the instantiation overhead. Daniel's comment above reinforces this concern to some extent, so we'll have to wait and compare benchmarks.
        Hide
        Doug Cutting added a comment -

        ThreadLocal?

        Show
        Doug Cutting added a comment - ThreadLocal?
        Hide
        Daniel Naber added a comment -

        Could you try changing the code to create a new object every time and then run your load test again? We original did that but it was slower, at least according to this commit comment from two years ago:

        "Don't re-create SimpleDateFormat objects, use static ones instead. Gives about a 2x performance increase in a micro benchmark."

        Show
        Daniel Naber added a comment - Could you try changing the code to create a new object every time and then run your load test again? We original did that but it was slower, at least according to this commit comment from two years ago: "Don't re-create SimpleDateFormat objects, use static ones instead. Gives about a 2x performance increase in a micro benchmark."
        Hide
        Mark Lassau added a comment -

        SimpleDateFormat javadoc:

        Date formats are not synchronized.
        It is recommended to create separate format instances for each thread.
        If multiple threads access a format concurrently, it must be synchronized externally.

        Show
        Mark Lassau added a comment - SimpleDateFormat javadoc : Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.
        Hide
        Mark Lassau added a comment -

        The following methods would potentially suffer contention as well depending on usage patterns of the particular app:

        • stringToTime()
        • dateToString()
        • timeToString()
        Show
        Mark Lassau added a comment - The following methods would potentially suffer contention as well depending on usage patterns of the particular app: stringToTime() dateToString() timeToString()

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development