Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 3.3, 4.0-ALPHA
    • Component/s: search
    • Labels:
      None

      Description

      This issue is dedicated to the performance of the grouping functionality.

      I've noticed that the code is not really performing on large indexes. Doing a search (q=:) with grouping on an index from around 5M documents took around one second on my local development machine. We had to support grouping on an index that holds around 50M documents per machine, so we made some changes and were able to happily serve that amount of documents. Patch will follow soon.

      1. SOLR-2205.patch
        9 kB
        Martijn van Groningen
      2. SOLR-2205.patch
        2 kB
        Yonik Seeley

        Activity

        Hide
        Yonik Seeley added a comment -

        Cool!
        Are these a different set of optimizations than those already proposed: SOLR-2069, SOLR-2068, SOLR-2070, and the rather unspecific SOLR-2071?

        Show
        Yonik Seeley added a comment - Cool! Are these a different set of optimizations than those already proposed: SOLR-2069 , SOLR-2068 , SOLR-2070 , and the rather unspecific SOLR-2071 ?
        Hide
        Martijn van Groningen added a comment -

        The code I initially wrote was on the pre-flex code base. So I took that code and made it work for the trunk. So someone should definitely check it out if all the changes I made are the right changes.

        I tested this patch out on my local machine and when doing a search (q=:) on an index that holds 10M documents, the searchtime was around 300 ms whereas the same query without the code changes had a searchtime of around 2.8 seconds. So that is +/- 9 times faster. These numbers are based on a basic search, so no facets or highlighting etc.

        I found out that the following piece of code took relatively a lot time to execute (if it was executed millions and millions of times, you started to notice):

        filler.fillValue(doc);
        groupMap.get(mval);
        

        This fragment is used in the TopGroupCollector and Phase2GroupCollector. I put some code in front of it the easily exclude documents that are not competitive. This code in both classes is cheaper then using the fragment above.

        Since I ported the code from pre-flex code I needed to make some changes to it and support grouping by function. The code I initially wrote only needed to support grouping on a field. Since the trunk also supports grouping by function query, I added two methods to DocValues and implemented these methods in three subclasses. I don't know if this particular change is good, but it works. I think that it would be really helpful is someone can give feedback on this particular change.

        Show
        Martijn van Groningen added a comment - The code I initially wrote was on the pre-flex code base. So I took that code and made it work for the trunk. So someone should definitely check it out if all the changes I made are the right changes. I tested this patch out on my local machine and when doing a search (q= : ) on an index that holds 10M documents, the searchtime was around 300 ms whereas the same query without the code changes had a searchtime of around 2.8 seconds. So that is +/- 9 times faster. These numbers are based on a basic search, so no facets or highlighting etc. I found out that the following piece of code took relatively a lot time to execute (if it was executed millions and millions of times, you started to notice): filler.fillValue(doc); groupMap.get(mval); This fragment is used in the TopGroupCollector and Phase2GroupCollector. I put some code in front of it the easily exclude documents that are not competitive. This code in both classes is cheaper then using the fragment above. Since I ported the code from pre-flex code I needed to make some changes to it and support grouping by function. The code I initially wrote only needed to support grouping on a field. Since the trunk also supports grouping by function query, I added two methods to DocValues and implemented these methods in three subclasses. I don't know if this particular change is good, but it works. I think that it would be really helpful is someone can give feedback on this particular change.
        Hide
        Martijn van Groningen added a comment -

        Are these a different set of optimizations than those already proposed: SOLR-2069, SOLR-2068, SOLR-2070, and the rather unspecific SOLR-2071?

        This patch is more a general improvement fix and I think wouldn't really fit in any of these issue.

        Show
        Martijn van Groningen added a comment - Are these a different set of optimizations than those already proposed: SOLR-2069 , SOLR-2068 , SOLR-2070 , and the rather unspecific SOLR-2071 ? This patch is more a general improvement fix and I think wouldn't really fit in any of these issue.
        Hide
        Martijn van Groningen added a comment - - edited

        the searchtime was around 300 ms whereas the same query without the code changes

        BTW this doesn't apply to the first search. The first search is slow, but by configuring query listeners the first user wouldn't notice.

        Show
        Martijn van Groningen added a comment - - edited the searchtime was around 300 ms whereas the same query without the code changes BTW this doesn't apply to the first search. The first search is slow , but by configuring query listeners the first user wouldn't notice.
        Hide
        Yonik Seeley added a comment -

        Oh, nice - I hadn't thought of checking if a doc is competitive before looking up it's group!

        The ord part is related to SOLR-2068 - I had worked out a per-segment algorithm a few months ago, but haven't had time to implement. It looks like this one operates at the top-level reader. That can be better for static indexes (those that don't change much), but isn't as good for NRT. Also, it looks like this will double memory usage (FieldCache per-segment as the native type, plus FieldCache at the top-level for the ords and string values). Something like that should be an option?

        // I believe that the JVM internally will represent a boolean inside a boolean array as a bit...

        Not that I've heard. It's internally represented as a byte[], so we would be better off using an OpenBitSet - or even better, a sparse set since the number of elements will be very small compared to the possible number of groups.

        Show
        Yonik Seeley added a comment - Oh, nice - I hadn't thought of checking if a doc is competitive before looking up it's group! The ord part is related to SOLR-2068 - I had worked out a per-segment algorithm a few months ago, but haven't had time to implement. It looks like this one operates at the top-level reader. That can be better for static indexes (those that don't change much), but isn't as good for NRT. Also, it looks like this will double memory usage (FieldCache per-segment as the native type, plus FieldCache at the top-level for the ords and string values). Something like that should be an option? // I believe that the JVM internally will represent a boolean inside a boolean array as a bit... Not that I've heard. It's internally represented as a byte[], so we would be better off using an OpenBitSet - or even better, a sparse set since the number of elements will be very small compared to the possible number of groups.
        Hide
        Martijn van Groningen added a comment - - edited

        ...... values). Something like that should be an option?

        So we'd have two different implementations of Phase2GroupCollector?

        Not that I've heard. It's internally represented as a byte[], so we would be better off using an OpenBitSet - or even better, a sparse set since the....

        Then we should do that. Maybe we should then move that part of the patch to SOLR-2068

        Show
        Martijn van Groningen added a comment - - edited ...... values). Something like that should be an option? So we'd have two different implementations of Phase2GroupCollector? Not that I've heard. It's internally represented as a byte[], so we would be better off using an OpenBitSet - or even better, a sparse set since the.... Then we should do that. Maybe we should then move that part of the patch to SOLR-2068
        Hide
        Yonik Seeley added a comment -

        I've committed the first part of this patch that compares with the smallest group first (before finding the group of the current doc). I also committed the addition of the ord methods to DocValues (we need something like that anyway).

        Here's an additional patch that removes the second (redundant) comparison with the lowest group (in addition to some other minor tweaks like removing the boolean).

        Show
        Yonik Seeley added a comment - I've committed the first part of this patch that compares with the smallest group first (before finding the group of the current doc). I also committed the addition of the ord methods to DocValues (we need something like that anyway). Here's an additional patch that removes the second (redundant) comparison with the lowest group (in addition to some other minor tweaks like removing the boolean).
        Hide
        Martijn van Groningen added a comment -

        Here's an additional patch that removes the second (redundant) comparison with the lowest group (in addition to some other minor tweaks like removing the boolean).

        Nice! Now it's even faster. Average search time is now 250 ms (measured on my test data set).

        I also committed the addition of the ord methods to DocValues

        Is see that the changes the DocValues impl in StrField class is not included. Is there a reason for that?

        Should I put the rest of my initial patch (changes to Phase2GroupCollector) to SOLR-2068?

        Show
        Martijn van Groningen added a comment - Here's an additional patch that removes the second (redundant) comparison with the lowest group (in addition to some other minor tweaks like removing the boolean). Nice! Now it's even faster. Average search time is now 250 ms (measured on my test data set). I also committed the addition of the ord methods to DocValues Is see that the changes the DocValues impl in StrField class is not included. Is there a reason for that? Should I put the rest of my initial patch (changes to Phase2GroupCollector) to SOLR-2068 ?
        Hide
        Yonik Seeley added a comment -

        Is see that the changes the DocValues impl in StrField class is not included. Is there a reason for that?

        Probably just user error... I'll check it out.

        Should I put the rest of my initial patch (changes to Phase2GroupCollector) to SOLR-2068?

        Sure!

        Show
        Yonik Seeley added a comment - Is see that the changes the DocValues impl in StrField class is not included. Is there a reason for that? Probably just user error... I'll check it out. Should I put the rest of my initial patch (changes to Phase2GroupCollector) to SOLR-2068 ? Sure!
        Hide
        Martijn van Groningen added a comment -

        I think the only thing to do for the issue is to also port the changes made in TopGroupCollector to TopGroupSortCollector. If that is done I think we can close this issue.

        Show
        Martijn van Groningen added a comment - I think the only thing to do for the issue is to also port the changes made in TopGroupCollector to TopGroupSortCollector. If that is done I think we can close this issue.
        Hide
        Yonik Seeley added a comment -

        I think the only thing to do for the issue is to also port the changes made in TopGroupCollector to TopGroupSortCollector.

        Is that a valid optimization for TopGroupSortCollector though? Given that the sorts are different, and the sort between groups is based on the first document by group, a document could not be competitive according to "sort", but could pop to the top of an existing group via "group.sort" and thus cause that group to move down in the rankings.

        This stuff is tricky enough, we still really need to develop some good random tests to verify any optimizations + corner cases.

        Show
        Yonik Seeley added a comment - I think the only thing to do for the issue is to also port the changes made in TopGroupCollector to TopGroupSortCollector. Is that a valid optimization for TopGroupSortCollector though? Given that the sorts are different, and the sort between groups is based on the first document by group, a document could not be competitive according to "sort", but could pop to the top of an existing group via "group.sort" and thus cause that group to move down in the rankings. This stuff is tricky enough, we still really need to develop some good random tests to verify any optimizations + corner cases.
        Hide
        Martijn van Groningen added a comment -

        I wrote that in a hurry, but group.sort should be used. But you've to keep track of the lowest first document of the top x groups in a separate field. If it less competitive then that you can neglect the incoming doc, right? Obviously then you need to update the lowest first document if the incoming doc is more competitive. So the code is different then what is in the TopGroupCollector.

        we still really need to develop some good random tests to verify any optimizations + corner case

        I agree with that, we need more tests that show that the grouping functionality is working properly. Should we put that in this issue or create an separate issue for this?

        Show
        Martijn van Groningen added a comment - I wrote that in a hurry, but group.sort should be used. But you've to keep track of the lowest first document of the top x groups in a separate field. If it less competitive then that you can neglect the incoming doc, right? Obviously then you need to update the lowest first document if the incoming doc is more competitive. So the code is different then what is in the TopGroupCollector. we still really need to develop some good random tests to verify any optimizations + corner case I agree with that, we need more tests that show that the grouping functionality is working properly. Should we put that in this issue or create an separate issue for this?
        Hide
        tom liu added a comment -

        Now, group Search do not support distributed query.

        anyone else, has already been meet this?

        Show
        tom liu added a comment - Now, group Search do not support distributed query. anyone else, has already been meet this?
        Hide
        Martijn van Groningen added a comment -

        Fixed. The proposed changes got into Solr 3x and 4.0-dev.

        Show
        Martijn van Groningen added a comment - Fixed. The proposed changes got into Solr 3x and 4.0-dev.

          People

          • Assignee:
            Unassigned
            Reporter:
            Martijn van Groningen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development