Lucene - Core
  1. Lucene - Core
  2. LUCENE-651

Poor performance race condition in FieldCacheImpl

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: None
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      Patch Available

      Description

      A race condition exists in FieldCacheImpl that causes a significant performance degradation if multiple threads concurrently request a value that is not yet cached. The degradation is particularly noticable in large indexes and when there a many concurent requests for the cached value.

      For the full discussion see the mailing list thread 'Poor performance "race condition" in FieldSortedHitQueue' (http://www.gossamer-threads.com/lists/lucene/java-user/38717).

      1. LUCENE-651.patch
        14 kB
        Oliver Hutchison

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        98d 5h 40m 1 Otis Gospodnetic 20/Nov/06 07:11
        Mark Thomas made changes -
        Workflow Default workflow, editable Closed status [ 12562353 ] jira [ 12583347 ]
        Mark Thomas made changes -
        Workflow jira [ 12381653 ] Default workflow, editable Closed status [ 12562353 ]
        Hide
        Otis Gospodnetic added a comment -

        For the record, this patch had a small mistake in it, causing memory leakage. The fix is in LUCENE-754.

        Show
        Otis Gospodnetic added a comment - For the record, this patch had a small mistake in it, causing memory leakage. The fix is in LUCENE-754 .
        Otis Gospodnetic made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Otis Gospodnetic added a comment -

        Committed, thanks Oliver!

        Show
        Otis Gospodnetic added a comment - Committed, thanks Oliver!
        Hide
        Otis Gospodnetic added a comment -

        Ah, yes, you mentioned that in that email thread, too. Ignore my comment.

        Show
        Otis Gospodnetic added a comment - Ah, yes, you mentioned that in that email thread, too. Ignore my comment.
        Hide
        Yonik Seeley added a comment -

        > Wouldn't synchronizing on readerCache be sufficient here?

        That would be deadly, esp to something like Solr. It would kill any ability to warm in the background as any index reader filling an entry would block all other readers from using the cache.

        Show
        Yonik Seeley added a comment - > Wouldn't synchronizing on readerCache be sufficient here? That would be deadly, esp to something like Solr. It would kill any ability to warm in the background as any index reader filling an entry would block all other readers from using the cache.
        Otis Gospodnetic made changes -
        Lucene Fields [Patch Available]
        Assignee Otis Gospodnetic [ otis ]
        Hide
        Otis Gospodnetic added a comment -

        FieldCache bit us in the ass recently, so I'm looking at other FieldCache issues.
        This one looks good to me
        (discussion that led to the patch is at http://www.gossamer-threads.com/lists/lucene/java-user/38717 )

        Question about this snippet:
        + synchronized (value) {
        + CreationPlaceholder progress = (CreationPlaceholder) value;
        + if (progress.value == null) {
        + progress.value = createValue(reader, key);
        + synchronized (readerCache)

        { + innerCache.put(key, progress.value); + }

        + }

        Is that synchronization on value really needed here? Wouldn't synchronizing on readerCache be sufficient here? Something like:

        + synchronized (readerCache) {
        + CreationPlaceholder progress = (CreationPlaceholder) value;
        + if (progress.value == null)

        { + progress.value = createValue(reader, key); + innerCache.put(key, progress.value); + }

        Or would that be too coarse? Maybe, I see createValue does a fair bit of work.

        I'll commit this shortly.

        Show
        Otis Gospodnetic added a comment - FieldCache bit us in the ass recently, so I'm looking at other FieldCache issues. This one looks good to me (discussion that led to the patch is at http://www.gossamer-threads.com/lists/lucene/java-user/38717 ) Question about this snippet: + synchronized (value) { + CreationPlaceholder progress = (CreationPlaceholder) value; + if (progress.value == null) { + progress.value = createValue(reader, key); + synchronized (readerCache) { + innerCache.put(key, progress.value); + } + } Is that synchronization on value really needed here? Wouldn't synchronizing on readerCache be sufficient here? Something like: + synchronized (readerCache) { + CreationPlaceholder progress = (CreationPlaceholder) value; + if (progress.value == null) { + progress.value = createValue(reader, key); + innerCache.put(key, progress.value); + } Or would that be too coarse? Maybe, I see createValue does a fair bit of work. I'll commit this shortly.
        Oliver Hutchison made changes -
        Field Original Value New Value
        Attachment LUCENE-651.patch [ 12338769 ]
        Hide
        Oliver Hutchison added a comment -

        The attached patch resolves this issue by forcing multiple concurrent requests to block and wait for a single thread to do the cached value generation.

        Show
        Oliver Hutchison added a comment - The attached patch resolves this issue by forcing multiple concurrent requests to block and wait for a single thread to do the cached value generation.
        Oliver Hutchison created issue -

          People

          • Assignee:
            Otis Gospodnetic
            Reporter:
            Oliver Hutchison
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development