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

        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 .
        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.
        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.
        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.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development