Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-7876

Support disabling ExitableDirectory wrapper when not needed

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.0, 5.1, 5.2, 5.2.1
    • Fix Version/s: 6.0
    • Component/s: None
    • Labels:
      None

      Description

      ExitableDirectoryReader can cause some overhead in Solr for some use cases, even when not really used. There should be a way of not using it when not needed.
      See http://search-lucene.com/m/l6pAi1HLrodLhNUd

      1. SOLR-7876.patch
        2 kB
        Yonik Seeley

        Activity

        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Thanks for finding this Tomas! This is a pretty bad performance regression for certain cases (facet.method=enum should be hit pretty hard too I imagine, along with any code that un-inverts a field).

        We should try to fix this for 5.3!

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Thanks for finding this Tomas! This is a pretty bad performance regression for certain cases (facet.method=enum should be hit pretty hard too I imagine, along with any code that un-inverts a field). We should try to fix this for 5.3!
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        We should try to fix this for 5.3!

        +1. I'm looking at SOLR-7875 first, but if after improving SolrQueryTimeoutImpl the performance is not still as good as the un-wrapped version I think we should have a way to skip the wrapping.

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - We should try to fix this for 5.3! +1. I'm looking at SOLR-7875 first, but if after improving SolrQueryTimeoutImpl the performance is not still as good as the un-wrapped version I think we should have a way to skip the wrapping.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        I tried implementing something that would use a different reader when a timeout isn't needed on a per-query basis, but I ran into issues
        From TermQuery / TermWeight:

              assert termStates.topReaderContext == ReaderUtil.getTopLevelContext(context) : "The top-reader used to create Weight (" + termStates.topReaderContext + ") is not the same as the current reader's top-reader (" + ReaderUtil.getTopLevelContext(context);
        

        createWeight takes a searcher, and the TermWeight implementation remembers the top reader context from that searcher, later comparing it against the readers used for scoring.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - I tried implementing something that would use a different reader when a timeout isn't needed on a per-query basis, but I ran into issues From TermQuery / TermWeight: assert termStates.topReaderContext == ReaderUtil.getTopLevelContext(context) : "The top-reader used to create Weight (" + termStates.topReaderContext + ") is not the same as the current reader's top-reader (" + ReaderUtil.getTopLevelContext(context); createWeight takes a searcher, and the TermWeight implementation remembers the top reader context from that searcher, later comparing it against the readers used for scoring.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        I got into the exact same problem, query must be rewritten with the same reader as the one used to search. Is it a good idea to chose different readers based on parameters for search? Seems like o.a.l.s.IndexSearcher is not designed to do it.

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - I got into the exact same problem, query must be rewritten with the same reader as the one used to search. Is it a good idea to chose different readers based on parameters for search? Seems like o.a.l.s.IndexSearcher is not designed to do it.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Only way I can think of to work around it is to use a thread local (like SolrRequestInfo) to return one reader or the other for searcher.getTopReaderContext(). I don't really like it, but it might be the only way.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Only way I can think of to work around it is to use a thread local (like SolrRequestInfo) to return one reader or the other for searcher.getTopReaderContext(). I don't really like it, but it might be the only way.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Bulk move to 5.4 after 5.3 release.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Bulk move to 5.4 after 5.3 release.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Here's a generic patch that escapes the ExitableDirectoryReader wrapping as soon as possible (in the fields() call). It should work, but I haven't had a chance to test the performance.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Here's a generic patch that escapes the ExitableDirectoryReader wrapping as soon as possible (in the fields() call). It should work, but I haven't had a chance to test the performance.
        Hide
        erickerickson Erick Erickson added a comment -

        Interesting. This patch fixes one of the new tests I made for SOLR-7990. TimeAllowedCachingTest.testQueryResults. Submitting a query that times out followed immediately by an identical one only with a bigger timeAllowed that does NOT time out used to still return partialResults=true but doesn't with this patch. Which makes some sense I think.

        Show
        erickerickson Erick Erickson added a comment - Interesting. This patch fixes one of the new tests I made for SOLR-7990 . TimeAllowedCachingTest.testQueryResults. Submitting a query that times out followed immediately by an identical one only with a bigger timeAllowed that does NOT time out used to still return partialResults=true but doesn't with this patch. Which makes some sense I think.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        Hmmm, that is interesting, and it raises red flags for me as I'm not sure why this patch would affect caching. ExitableDirectoryReader and friends delegate cache core keys to the underlying readers. Any theories / clues?

        Show
        yseeley@gmail.com Yonik Seeley added a comment - Hmmm, that is interesting, and it raises red flags for me as I'm not sure why this patch would affect caching. ExitableDirectoryReader and friends delegate cache core keys to the underlying readers. Any theories / clues?
        Hide
        erickerickson Erick Erickson added a comment -

        The test that it cured was this sequence:

        > send a query that times out (no fq clauses)
        > send the same query that does not time out (still no fq clauses)
        ***>test the second response to see if partialResults==true

        It's the *** part the suddenly succeeded after this patch. Prior to this patch partialResults was true, now it's just not returned in the second response at all, which makes sense on the surface if your patch bails out from that wrapper earlier. This test was returning the correct number of docs before, it was just that the partialResults was incorrectly returned.

        The tests that go against the filterCache are still failing though, so it's not like this patch magically fixed any of the caching problems.

        So I'd guess not returning the partialResults flag makes more sense in terms of this patch?

        Haven't updated the test patch yet, just tried it locally.

        Show
        erickerickson Erick Erickson added a comment - The test that it cured was this sequence: > send a query that times out (no fq clauses) > send the same query that does not time out (still no fq clauses) ***>test the second response to see if partialResults==true It's the *** part the suddenly succeeded after this patch. Prior to this patch partialResults was true, now it's just not returned in the second response at all, which makes sense on the surface if your patch bails out from that wrapper earlier. This test was returning the correct number of docs before, it was just that the partialResults was incorrectly returned. The tests that go against the filterCache are still failing though, so it's not like this patch magically fixed any of the caching problems. So I'd guess not returning the partialResults flag makes more sense in terms of this patch? Haven't updated the test patch yet, just tried it locally.
        Hide
        erickerickson Erick Erickson added a comment -

        OK, on a second look it was a bad test.

        copy/pasted the original code for the first call with timeAllowed=1 that looked at partialResults but didn't test for null. Just saw the error and didn't read it carefully.

        assertFalse("Should NOT have partial results", (Boolean) (header.get("partialResults"));

        When timeAllowed=1000, get("partialResults") returns null.....

        So this patch has no effect whatsoever on the new tests I wrote. I removed the linked-to.

        Show
        erickerickson Erick Erickson added a comment - OK, on a second look it was a bad test. copy/pasted the original code for the first call with timeAllowed=1 that looked at partialResults but didn't test for null. Just saw the error and didn't read it carefully. assertFalse("Should NOT have partial results", (Boolean) (header.get("partialResults")); When timeAllowed=1000, get("partialResults") returns null..... So this patch has no effect whatsoever on the new tests I wrote. I removed the linked-to.
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        OK, I did some ad-hoc performance testing on near worst case range query on id field and saw an 8% performance improvement on my box.
        I'll plan on committing this shortly.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - OK, I did some ad-hoc performance testing on near worst case range query on id field and saw an 8% performance improvement on my box. I'll plan on committing this shortly.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1700276 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1700276 ]

        SOLR-7876: exit ExitableDirectoryReader wrapper if timeout is not enabled

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1700276 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1700276 ] SOLR-7876 : exit ExitableDirectoryReader wrapper if timeout is not enabled
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Yonik Seeley, Any reason why this couldn't be backported?

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Yonik Seeley , Any reason why this couldn't be backported?
        Hide
        yseeley@gmail.com Yonik Seeley added a comment -

        The QueryTimeout interface was put into Lucene when this feature (SOLR-5986) was committed, and I changed it's signature.

        Show
        yseeley@gmail.com Yonik Seeley added a comment - The QueryTimeout interface was put into Lucene when this feature ( SOLR-5986 ) was committed, and I changed it's signature.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Oh, I didn't notice you where changing the interface. Makes sense then.

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Oh, I didn't notice you where changing the interface. Makes sense then.

          People

          • Assignee:
            yseeley@gmail.com Yonik Seeley
            Reporter:
            tomasflobbe Tomás Fernández Löbbe
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development