Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2, 6.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      FacetsAccumulator and FacetRequest expose too many things to users, even when they are not needed, e.g. complements and partitions. Also, Aggregator is created per-FacetRequest, while in fact applied per category list. This is confusing, because if you want to do two aggregations, e.g. count and sum-score, you need to separate the two dimensions into two different category lists at indexing time.

      It's not so easy to refactor everything in one go, since there's a lot of code involved. So in this issue I will:

      • Remove complements from FacetRequest. It is only relevant to CountFacetRequest anyway. In the future, it should be a special Accumulator.
      • Make FacetsAccumulator concrete class, and StandardFacetsAccumulator extend it and handles all the stuff that's relevant to sampling, complements and partitions. Gradually, these things will be migrated to the new API, and hopefully StandardFacetsAccumulator will go away.
      • Aggregator is per-document. I could not break its API b/c some features (e.g. complement) depend on it. So rather I created a new FacetsAggregator, with a bulk, per-segment, API. So far migrated Counting and SumScore to that API.
        • In the new API, you need to override FacetsAccumulator to define an Aggregator for use, the default is CountingFacetsAggregator.
      • Started to refactor FacetResultsHandler, which its API was guided by the use of partitions. I added a simple compute(FacetArrays) to it, which by default delegates to the nasty API, but overridden by specific classes. This will get cleaned further along too.
      • FacetRequest has a .getValueOf() which resolves an ordinal to its value (i.e. which of the two arrays to use). I added FacetRequest.FacetArraysSource and specialize when they are INT or FLOAT, creating a special FacetResultsHandler which does not go back to FR.getValueOf for every ordinal. I think that we can migrate other FacetResultsHandlers to behave like that ... at the expense of code duplication.
        • I also added a TODO to get rid of getValueOf entirely .. will be done separately.
      • Got rid of CountingFacetsCollector and StandardFacetsCollector in favor of a single FacetsCollector which collects matching documents, and optionally scores, per-segment. I wrote a migration class from these per-segment MatchingDocs to ScoredDocIDs (which is global), so that the rest of the code works, but the new code works w/ the optimized per-segment API. I hope performance is still roughly the same w/ these changes too.

      There will be follow-on issues to migrate more features to the new API, and more cleanups ...

      1. LUCENE-4757.patch
        260 kB
        Shai Erera
      2. LUCENE-4757.patch
        245 kB
        Shai Erera
      3. LUCENE-4757.patch
        234 kB
        Shai Erera
      4. LUCENE-4757.patch
        234 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Patch with the mentioned changes. All tests pass and 'ant precommit' is happy. As I wrote in CHANGES, whoever uses FacetsCollector.create() is not affected by these API changes. Only those that wrote an Aggregator, Accumulator etc. (I doubt there are any).

        Show
        Shai Erera added a comment - Patch with the mentioned changes. All tests pass and 'ant precommit' is happy. As I wrote in CHANGES, whoever uses FacetsCollector.create() is not affected by these API changes. Only those that wrote an Aggregator, Accumulator etc. (I doubt there are any).
        Hide
        Shai Erera added a comment -

        Patch consolidates Int/FloatFacetResultsHandler to extend an abstract DepthOneFRH and batch-fill the values in the PQ. Reduces some major code duplication, as these two are only different by which value they set on the result node (int/float).

        Show
        Shai Erera added a comment - Patch consolidates Int/FloatFacetResultsHandler to extend an abstract DepthOneFRH and batch-fill the values in the PQ. Reduces some major code duplication, as these two are only different by which value they set on the result node (int/float).
        Hide
        Michael McCandless added a comment -

        I tested perf of last patch on wikibig, with 7 facet dims:

                            Task    QPS base      StdDev    QPS comp      StdDev                Pct diff
                          IntNRQ        4.15      (2.6%)        3.88      (2.8%)   -6.4% ( -11% -   -1%)
                        HighTerm       22.40      (3.0%)       21.05      (3.3%)   -6.0% ( -11% -    0%)
                         Prefix3       14.92      (2.3%)       14.15      (2.4%)   -5.2% (  -9% -    0%)
                         MedTerm       53.74      (2.5%)       51.02      (2.9%)   -5.1% ( -10% -    0%)
                       OrHighLow       19.23      (2.8%)       18.35      (3.0%)   -4.6% ( -10% -    1%)
                       OrHighMed       18.62      (2.8%)       17.77      (3.0%)   -4.6% ( -10% -    1%)
                      OrHighHigh        9.79      (3.0%)        9.35      (3.1%)   -4.5% ( -10% -    1%)
                        Wildcard       30.48      (1.7%)       29.44      (2.1%)   -3.4% (  -7% -    0%)
                         LowTerm      114.24      (1.6%)      112.06      (1.8%)   -1.9% (  -5% -    1%)
                     AndHighHigh       23.91      (0.8%)       23.54      (1.3%)   -1.5% (  -3% -    0%)
                          Fuzzy1       48.93      (2.0%)       48.30      (2.0%)   -1.3% (  -5% -    2%)
                          Fuzzy2       56.09      (3.0%)       55.38      (2.4%)   -1.3% (  -6% -    4%)
                         Respell       46.99      (3.7%)       46.39      (2.9%)   -1.3% (  -7% -    5%)
                       MedPhrase      120.51      (5.7%)      119.16      (6.0%)   -1.1% ( -12% -   11%)
                HighSloppyPhrase        0.94      (4.5%)        0.93      (6.1%)   -1.1% ( -11% -    9%)
                 MedSloppyPhrase       26.59      (1.4%)       26.37      (2.4%)   -0.8% (  -4% -    3%)
                       LowPhrase       21.67      (5.6%)       21.52      (6.1%)   -0.7% ( -11% -   11%)
                      HighPhrase       17.80     (10.0%)       17.70     (10.7%)   -0.6% ( -19% -   22%)
                      AndHighMed      108.97      (0.6%)      108.48      (0.9%)   -0.4% (  -1% -    1%)
                 LowSloppyPhrase       20.81      (2.0%)       20.74      (2.2%)   -0.3% (  -4% -    3%)
                     MedSpanNear       29.10      (1.3%)       29.03      (1.1%)   -0.2% (  -2% -    2%)
                    HighSpanNear        3.57      (1.6%)        3.57      (1.3%)   -0.0% (  -2% -    2%)
                     LowSpanNear        8.46      (2.2%)        8.46      (2.0%)    0.0% (  -4% -    4%)
                      AndHighLow      665.03      (1.5%)      668.55      (2.0%)    0.5% (  -2% -    4%)
        

        Looks like things got a bit slower ... not sure why.

        Show
        Michael McCandless added a comment - I tested perf of last patch on wikibig, with 7 facet dims: Task QPS base StdDev QPS comp StdDev Pct diff IntNRQ 4.15 (2.6%) 3.88 (2.8%) -6.4% ( -11% - -1%) HighTerm 22.40 (3.0%) 21.05 (3.3%) -6.0% ( -11% - 0%) Prefix3 14.92 (2.3%) 14.15 (2.4%) -5.2% ( -9% - 0%) MedTerm 53.74 (2.5%) 51.02 (2.9%) -5.1% ( -10% - 0%) OrHighLow 19.23 (2.8%) 18.35 (3.0%) -4.6% ( -10% - 1%) OrHighMed 18.62 (2.8%) 17.77 (3.0%) -4.6% ( -10% - 1%) OrHighHigh 9.79 (3.0%) 9.35 (3.1%) -4.5% ( -10% - 1%) Wildcard 30.48 (1.7%) 29.44 (2.1%) -3.4% ( -7% - 0%) LowTerm 114.24 (1.6%) 112.06 (1.8%) -1.9% ( -5% - 1%) AndHighHigh 23.91 (0.8%) 23.54 (1.3%) -1.5% ( -3% - 0%) Fuzzy1 48.93 (2.0%) 48.30 (2.0%) -1.3% ( -5% - 2%) Fuzzy2 56.09 (3.0%) 55.38 (2.4%) -1.3% ( -6% - 4%) Respell 46.99 (3.7%) 46.39 (2.9%) -1.3% ( -7% - 5%) MedPhrase 120.51 (5.7%) 119.16 (6.0%) -1.1% ( -12% - 11%) HighSloppyPhrase 0.94 (4.5%) 0.93 (6.1%) -1.1% ( -11% - 9%) MedSloppyPhrase 26.59 (1.4%) 26.37 (2.4%) -0.8% ( -4% - 3%) LowPhrase 21.67 (5.6%) 21.52 (6.1%) -0.7% ( -11% - 11%) HighPhrase 17.80 (10.0%) 17.70 (10.7%) -0.6% ( -19% - 22%) AndHighMed 108.97 (0.6%) 108.48 (0.9%) -0.4% ( -1% - 1%) LowSloppyPhrase 20.81 (2.0%) 20.74 (2.2%) -0.3% ( -4% - 3%) MedSpanNear 29.10 (1.3%) 29.03 (1.1%) -0.2% ( -2% - 2%) HighSpanNear 3.57 (1.6%) 3.57 (1.3%) -0.0% ( -2% - 2%) LowSpanNear 8.46 (2.2%) 8.46 (2.0%) 0.0% ( -4% - 4%) AndHighLow 665.03 (1.5%) 668.55 (2.0%) 0.5% ( -2% - 4%) Looks like things got a bit slower ... not sure why.
        Hide
        Shai Erera added a comment -

        There are few abstractions that were added (compared to CountingFC), but I don't see why they will slow things down:

        • FacetsCollector.getFacetResult() delegates to FacetsAccumulator vs. CountingFC which does the work on its own. Still, that's one method call for computing all the results.
        • FacetsAccumulator calls FacetsAggregator to aggregate (count) the facets. That's one API call, and CountingFC also calls a method to do that. One difference could be JIT - in CountingFC, the method that's called is private, while FacetsAggregator is an interface. Since we don't know when JIT kicks in and when not, we can't tell for sure that it's the cause. Still, I could make CountingFacetsAggregator and FastCountingFacetsAggregator final, if that may help. What do you think?
        • FacetsAccumulator calls FacetResultsHandler to compute the top-K facets, where CountingFC computed on its own (but still used a PQ class). That too is one method call, but same like FacetsAggregator, may not be JIT-able. I made some improvements (patch to follow shortly) where Int/FloatFacetResultsHandler are made final and FRHandler now takes FacetArrays in its ctor, which means they can access final class members only. Perhaps that will help. It makes FacetResultsHandler stateful, but the code now anyway initializes a new instance per FacetRequest (which is a cheap instance).

        The code now is more "structured" and clean. I can write a FastCountingFacetsAccumulator which will go back to doing aggregation + top-K computation on its own, privately. But that starts to get ugly ... not sure it's worth it for only 6%?

        Show
        Shai Erera added a comment - There are few abstractions that were added (compared to CountingFC), but I don't see why they will slow things down: FacetsCollector.getFacetResult() delegates to FacetsAccumulator vs. CountingFC which does the work on its own. Still, that's one method call for computing all the results. FacetsAccumulator calls FacetsAggregator to aggregate (count) the facets. That's one API call, and CountingFC also calls a method to do that. One difference could be JIT - in CountingFC, the method that's called is private, while FacetsAggregator is an interface. Since we don't know when JIT kicks in and when not, we can't tell for sure that it's the cause. Still, I could make CountingFacetsAggregator and FastCountingFacetsAggregator final, if that may help. What do you think? FacetsAccumulator calls FacetResultsHandler to compute the top-K facets, where CountingFC computed on its own (but still used a PQ class). That too is one method call, but same like FacetsAggregator, may not be JIT-able. I made some improvements (patch to follow shortly) where Int/FloatFacetResultsHandler are made final and FRHandler now takes FacetArrays in its ctor, which means they can access final class members only. Perhaps that will help. It makes FacetResultsHandler stateful, but the code now anyway initializes a new instance per FacetRequest (which is a cheap instance). The code now is more "structured" and clean. I can write a FastCountingFacetsAccumulator which will go back to doing aggregation + top-K computation on its own, privately. But that starts to get ugly ... not sure it's worth it for only 6%?
        Hide
        Michael McCandless added a comment -

        I don't think the extra method call (single method call, for all of faceting) is the issue.

        I think it could be the added abstraction for DocsOnlyCollector? Ie, now it's DocsOnlyCollector subclassing FacetsCollector (while before it was just CountingFacetsCollector), and in its collect method it must deref seg.matchingDocs, and seg.totalHits, but before it was just bits reference?

        Show
        Michael McCandless added a comment - I don't think the extra method call (single method call, for all of faceting) is the issue. I think it could be the added abstraction for DocsOnlyCollector? Ie, now it's DocsOnlyCollector subclassing FacetsCollector (while before it was just CountingFacetsCollector), and in its collect method it must deref seg.matchingDocs, and seg.totalHits, but before it was just bits reference?
        Hide
        Shai Erera added a comment -

        Really, you think that deref-ing can affect it that much? I can put the bits on the Collector instance, but can't w/ totalHits ... if Collector had something like close()/done(), then we could update MatchingDocs on every setNextReader (the prev MD) and on close/done update the last one ...

        I noticed that while I coded, but didn't think it's going to be such an issue ... hmm, maybe we can "update" the last one on getMatchingDocs / getFacetResults?

        Show
        Shai Erera added a comment - Really, you think that deref-ing can affect it that much? I can put the bits on the Collector instance, but can't w/ totalHits ... if Collector had something like close()/done(), then we could update MatchingDocs on every setNextReader (the prev MD) and on close/done update the last one ... I noticed that while I coded, but didn't think it's going to be such an issue ... hmm, maybe we can "update" the last one on getMatchingDocs / getFacetResults?
        Hide
        Shai Erera added a comment -

        Patch tracks bits, totalHits and scores by each Collector separately, and a MatchingDocs is created on setNextReader and finish (called by getFacetResults, reset and getMatchingDocs). Also, made the private Collectors final as well as their methods. Also MatchingDocs members are final too.

        I wonder if that will change anything. We didn't test the effects of the previous patch (makng more stuff final), but I don't think those matter much...

        Show
        Shai Erera added a comment - Patch tracks bits, totalHits and scores by each Collector separately, and a MatchingDocs is created on setNextReader and finish (called by getFacetResults, reset and getMatchingDocs). Also, made the private Collectors final as well as their methods. Also MatchingDocs members are final too. I wonder if that will change anything. We didn't test the effects of the previous patch (makng more stuff final), but I don't think those matter much...
        Hide
        Shai Erera added a comment -

        Patch separates partitions from FacetResultsHandler. All partitions stuff was moved to PartitionsFRH under o.a.l.facet.partitions.search.

        Show
        Shai Erera added a comment - Patch separates partitions from FacetResultsHandler. All partitions stuff was moved to PartitionsFRH under o.a.l.facet.partitions.search.
        Hide
        Michael McCandless added a comment -

        Perf results on full en wiki index for last patch:

                            Task    QPS base      StdDev    QPS comp      StdDev                Pct diff
                          IntNRQ        4.14      (2.7%)        3.86      (2.6%)   -6.9% ( -11% -   -1%)
                        HighTerm       22.41      (2.8%)       20.95      (3.3%)   -6.5% ( -12% -    0%)
                         MedTerm       53.33      (2.4%)       50.23      (2.8%)   -5.8% ( -10% -    0%)
                         Prefix3       14.82      (2.7%)       13.97      (2.2%)   -5.8% ( -10% -    0%)
                       OrHighLow       19.22      (2.9%)       18.16      (2.8%)   -5.5% ( -10% -    0%)
                       OrHighMed       18.62      (2.9%)       17.62      (2.7%)   -5.4% ( -10% -    0%)
                      OrHighHigh        9.80      (3.1%)        9.28      (2.9%)   -5.3% ( -10% -    0%)
                        Wildcard       29.91      (1.7%)       28.74      (1.6%)   -3.9% (  -7% -    0%)
                         LowTerm      111.45      (2.0%)      109.14      (1.3%)   -2.1% (  -5% -    1%)
                     AndHighHigh       23.73      (1.1%)       23.24      (1.1%)   -2.0% (  -4% -    0%)
                       MedPhrase      115.26      (5.8%)      113.02      (5.7%)   -1.9% ( -12% -   10%)
                          Fuzzy1       47.08      (2.2%)       46.66      (2.3%)   -0.9% (  -5% -    3%)
                      HighPhrase       17.55     (10.3%)       17.40     (10.3%)   -0.9% ( -19% -   21%)
                      AndHighLow      601.66      (2.4%)      597.32      (1.7%)   -0.7% (  -4% -    3%)
                HighSloppyPhrase        0.94      (7.1%)        0.93      (6.3%)   -0.6% ( -13% -   13%)
                      AndHighMed      105.65      (1.4%)      105.15      (1.0%)   -0.5% (  -2% -    1%)
                       LowPhrase       21.21      (6.1%)       21.12      (6.1%)   -0.4% ( -11% -   12%)
                         Respell       46.16      (3.9%)       45.96      (4.4%)   -0.4% (  -8% -    8%)
                          Fuzzy2       53.16      (3.1%)       52.95      (3.2%)   -0.4% (  -6% -    6%)
                 MedSloppyPhrase       26.11      (2.2%)       26.02      (1.9%)   -0.3% (  -4% -    3%)
                 LowSloppyPhrase       20.53      (2.8%)       20.47      (2.4%)   -0.3% (  -5% -    5%)
                    HighSpanNear        3.53      (1.9%)        3.53      (1.8%)   -0.0% (  -3% -    3%)
                     MedSpanNear       28.56      (1.9%)       28.56      (2.1%)   -0.0% (  -3% -    4%)
                     LowSpanNear        8.31      (2.6%)        8.34      (2.8%)    0.3% (  -5% -    5%)
        

        Net/net no faster (maybe a bit slower!) so I think we should just to back to the previous patch?

        Show
        Michael McCandless added a comment - Perf results on full en wiki index for last patch: Task QPS base StdDev QPS comp StdDev Pct diff IntNRQ 4.14 (2.7%) 3.86 (2.6%) -6.9% ( -11% - -1%) HighTerm 22.41 (2.8%) 20.95 (3.3%) -6.5% ( -12% - 0%) MedTerm 53.33 (2.4%) 50.23 (2.8%) -5.8% ( -10% - 0%) Prefix3 14.82 (2.7%) 13.97 (2.2%) -5.8% ( -10% - 0%) OrHighLow 19.22 (2.9%) 18.16 (2.8%) -5.5% ( -10% - 0%) OrHighMed 18.62 (2.9%) 17.62 (2.7%) -5.4% ( -10% - 0%) OrHighHigh 9.80 (3.1%) 9.28 (2.9%) -5.3% ( -10% - 0%) Wildcard 29.91 (1.7%) 28.74 (1.6%) -3.9% ( -7% - 0%) LowTerm 111.45 (2.0%) 109.14 (1.3%) -2.1% ( -5% - 1%) AndHighHigh 23.73 (1.1%) 23.24 (1.1%) -2.0% ( -4% - 0%) MedPhrase 115.26 (5.8%) 113.02 (5.7%) -1.9% ( -12% - 10%) Fuzzy1 47.08 (2.2%) 46.66 (2.3%) -0.9% ( -5% - 3%) HighPhrase 17.55 (10.3%) 17.40 (10.3%) -0.9% ( -19% - 21%) AndHighLow 601.66 (2.4%) 597.32 (1.7%) -0.7% ( -4% - 3%) HighSloppyPhrase 0.94 (7.1%) 0.93 (6.3%) -0.6% ( -13% - 13%) AndHighMed 105.65 (1.4%) 105.15 (1.0%) -0.5% ( -2% - 1%) LowPhrase 21.21 (6.1%) 21.12 (6.1%) -0.4% ( -11% - 12%) Respell 46.16 (3.9%) 45.96 (4.4%) -0.4% ( -8% - 8%) Fuzzy2 53.16 (3.1%) 52.95 (3.2%) -0.4% ( -6% - 6%) MedSloppyPhrase 26.11 (2.2%) 26.02 (1.9%) -0.3% ( -4% - 3%) LowSloppyPhrase 20.53 (2.8%) 20.47 (2.4%) -0.3% ( -5% - 5%) HighSpanNear 3.53 (1.9%) 3.53 (1.8%) -0.0% ( -3% - 3%) MedSpanNear 28.56 (1.9%) 28.56 (2.1%) -0.0% ( -3% - 4%) LowSpanNear 8.31 (2.6%) 8.34 (2.8%) 0.3% ( -5% - 5%) Net/net no faster (maybe a bit slower!) so I think we should just to back to the previous patch?
        Hide
        Shai Erera added a comment -

        I don't know if we should go back to the previous version. The current version allowed making MatchingDocs members final, and whatever we see in perf, the current Collectors work w/ local refs, rather than derefing .. that cannot be slower !?

        I think that I'll commit this issue as-is. There are a lot of changes, the API is cleaner and I want to start migrating other features to the new API.

        Show
        Shai Erera added a comment - I don't know if we should go back to the previous version. The current version allowed making MatchingDocs members final, and whatever we see in perf, the current Collectors work w/ local refs, rather than derefing .. that cannot be slower !? I think that I'll commit this issue as-is. There are a lot of changes, the API is cleaner and I want to start migrating other features to the new API.
        Hide
        Michael McCandless added a comment -

        OK it's fine to keep the current one ... it's entirely possible the delta is noise. It is hard to believe it's slower but hotspot is surprising

        Show
        Michael McCandless added a comment - OK it's fine to keep the current one ... it's entirely possible the delta is noise. It is hard to believe it's slower but hotspot is surprising
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Shai Erera
        http://svn.apache.org/viewvc?view=revision&revision=1443736

        LUCENE-4757: cleanup FacetsAccumulator API

        Show
        Commit Tag Bot added a comment - [trunk commit] Shai Erera http://svn.apache.org/viewvc?view=revision&revision=1443736 LUCENE-4757 : cleanup FacetsAccumulator API
        Hide
        Shai Erera added a comment -

        Committed to trunk. Will wait for Robert to port DV 2.0 changes to 4x before I port these changes. Keeping the issue open until then.

        Show
        Shai Erera added a comment - Committed to trunk. Will wait for Robert to port DV 2.0 changes to 4x before I port these changes. Keeping the issue open until then.
        Hide
        Shai Erera added a comment -

        Ported to 4x.

        Show
        Shai Erera added a comment - Ported to 4x.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Shai Erera
        http://svn.apache.org/viewvc?view=revision&revision=1443839

        LUCENE-4757: cleanup FacetsAccumulator API

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Shai Erera http://svn.apache.org/viewvc?view=revision&revision=1443839 LUCENE-4757 : cleanup FacetsAccumulator API
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development