Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: modules/grouping
    • Labels:
      None
    • Lucene Fields:
      Patch Available

      Description

      Although IDV is not yet finalized (More particular the SortedSource). I think we already can discuss / investigate implementing grouping by IDV.

      1. LUCENE-3496.patch
        93 kB
        Martijn van Groningen
      2. LUCENE-3496.patch
        51 kB
        Martijn van Groningen
      3. LUCENE-3496.patch
        56 kB
        Martijn van Groningen
      4. LUCENE-3496.patch
        24 kB
        Martijn van Groningen
      5. LUCENE-3496.patch
        18 kB
        Martijn van Groningen
      6. LUCENE-3496.patch
        12 kB
        Martijn van Groningen
      7. LUCENE-3496.patch
        11 kB
        Simon Willnauer
      8. LUCENE-3496.patch
        7 kB
        Martijn van Groningen

        Issue Links

          Activity

          Hide
          Martijn van Groningen added a comment - - edited

          Attached initial IDV AbstractAllGroupsCollector implementation.

          Show
          Martijn van Groningen added a comment - - edited Attached initial IDV AbstractAllGroupsCollector implementation.
          Hide
          Simon Willnauer added a comment -

          hey martjin, do yo have some benchmark results for IDV showing the impact? I would guess that depending on the type ie. var vs. fixed size you could see some perf hits. We might need to round up the packedint sizes inside IDV for lookup perf and pay the prize for a slightly larger size on disk / memory.

          Show
          Simon Willnauer added a comment - hey martjin, do yo have some benchmark results for IDV showing the impact? I would guess that depending on the type ie. var vs. fixed size you could see some perf hits. We might need to round up the packedint sizes inside IDV for lookup perf and pay the prize for a slightly larger size on disk / memory.
          Hide
          Martijn van Groningen added a comment -

          Up until now I only checked the difference between BYTES_VAR_SORTED and BYTES_FIXED_SORTED. The search time and memory usage seem to be more or less the same. I executed all queries on 30M index and the performance with grouping by IDV is somewhere between 500 ms and 550 ms. The same test with grouping by indexed values have search times somewhere between 300 ms and 330 ms.

          We might need to round up the packedint sizes inside IDV for lookup perf and pay the prize for a slightly larger size on disk / memory.

          I think we need this. Just like we have with FC#getTermsIndex(). A boolean option that either uses PackedInt or DirectInt. I think the ~200 ms difference is b/c IDV is using PackedInt and FC uses DirectInt.

          Show
          Martijn van Groningen added a comment - Up until now I only checked the difference between BYTES_VAR_SORTED and BYTES_FIXED_SORTED. The search time and memory usage seem to be more or less the same. I executed all queries on 30M index and the performance with grouping by IDV is somewhere between 500 ms and 550 ms. The same test with grouping by indexed values have search times somewhere between 300 ms and 330 ms. We might need to round up the packedint sizes inside IDV for lookup perf and pay the prize for a slightly larger size on disk / memory. I think we need this. Just like we have with FC#getTermsIndex(). A boolean option that either uses PackedInt or DirectInt. I think the ~200 ms difference is b/c IDV is using PackedInt and FC uses DirectInt.
          Hide
          Simon Willnauer added a comment -

          martjin, here is a patch that forces the sorted variants to use fixed size packed ints. can you check if the perf changes?

          Show
          Simon Willnauer added a comment - martjin, here is a patch that forces the sorted variants to use fixed size packed ints. can you check if the perf changes?
          Hide
          Martijn van Groningen added a comment - - edited

          Simon, this looks great!
          The performance is much better now. An average the search time for grouping by BYTES_VAR_SORTED is 329 ms and BYTES_FIXED_SORTED is 338 ms. The average search time for grouping by indexed values is 310 ms. So grouping by IDV is pretty close to grouping by indexed values!

          Show
          Martijn van Groningen added a comment - - edited Simon, this looks great! The performance is much better now. An average the search time for grouping by BYTES_VAR_SORTED is 329 ms and BYTES_FIXED_SORTED is 338 ms. The average search time for grouping by indexed values is 310 ms. So grouping by IDV is pretty close to grouping by indexed values!
          Hide
          Yonik Seeley added a comment -

          Wow, only a 9% performance penalty for not keeping the values in the JVM heap? That's quite an achievement (and almost hard to believe, unless the base query were really expensive or something). Nice job!

          Show
          Yonik Seeley added a comment - Wow, only a 9% performance penalty for not keeping the values in the JVM heap? That's quite an achievement (and almost hard to believe, unless the base query were really expensive or something). Nice job!
          Hide
          Simon Willnauer added a comment -

          Wow, only a 9% performance penalty for not keeping the values in the JVM heap? That's quite an achievement (and almost hard to believe, unless the base query were really expensive or something). Nice job!

          wait! this is in memory - martjin can you test with getDirectSource() instead of getSource()?

          Show
          Simon Willnauer added a comment - Wow, only a 9% performance penalty for not keeping the values in the JVM heap? That's quite an achievement (and almost hard to believe, unless the base query were really expensive or something). Nice job! wait! this is in memory - martjin can you test with getDirectSource() instead of getSource()?
          Hide
          Martijn van Groningen added a comment -

          getDirectSource() search times:

          • BYTES_VAR_SORTED: 455 ms
          • BYTES_FIXED_SORTED: 713 ms

          I noticed that with BYTES_VAR_SORTED the memory usage varied a lot during the test execution from around 10MB to 320MB. The memory usage for BYTES_FIXED_SORTED is stable around 10MB.

          I'll try to use luceneutil and modify it to benchmark grouping by IDV, so others can run performance tests as well.

          Show
          Martijn van Groningen added a comment - getDirectSource() search times: BYTES_VAR_SORTED: 455 ms BYTES_FIXED_SORTED: 713 ms I noticed that with BYTES_VAR_SORTED the memory usage varied a lot during the test execution from around 10MB to 320MB. The memory usage for BYTES_FIXED_SORTED is stable around 10MB. I'll try to use luceneutil and modify it to benchmark grouping by IDV, so others can run performance tests as well.
          Hide
          Martijn van Groningen added a comment -

          Minor update to IndexDocValuesAllGroupsCollector to use disk resident IDV.

          Show
          Martijn van Groningen added a comment - Minor update to IndexDocValuesAllGroupsCollector to use disk resident IDV.
          Hide
          Michael McCandless added a comment -

          It's great that rounding up to 8/16/32/64 bit width resolved the performance problem!

          I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time?

          Show
          Michael McCandless added a comment - It's great that rounding up to 8/16/32/64 bit width resolved the performance problem! I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time?
          Hide
          Simon Willnauer added a comment - - edited

          I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time?

          I think we should do that. Maybe via IWC?

          I noticed that with BYTES_VAR_SORTED the memory usage varied a lot during the test execution from around 10MB to 320MB. The memory usage for BYTES_FIXED_SORTED is stable around 10MB.

          yeah, the VAR_SORTED variant loads the ords in memory which is wrong though. However, LUCENE-3507 is adressing this issue.

          one thing I noticed about the patch is that you consider 0 ords as null values which is not the case in IDV. All documents do have values, there is nothing like missing values. VAR_BYTES will return a 0-length byte and FIXED_BYTES will return a fixed-length 0-byte array. Numeric variants default to 0 and 0.0 receptively.

          Show
          Simon Willnauer added a comment - - edited I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time? I think we should do that. Maybe via IWC? I noticed that with BYTES_VAR_SORTED the memory usage varied a lot during the test execution from around 10MB to 320MB. The memory usage for BYTES_FIXED_SORTED is stable around 10MB. yeah, the VAR_SORTED variant loads the ords in memory which is wrong though. However, LUCENE-3507 is adressing this issue. one thing I noticed about the patch is that you consider 0 ords as null values which is not the case in IDV. All documents do have values, there is nothing like missing values. VAR_BYTES will return a 0-length byte and FIXED_BYTES will return a fixed-length 0-byte array. Numeric variants default to 0 and 0.0 receptively.
          Hide
          Michael McCandless added a comment -

          I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time?

          I think we should do that. Maybe via IWC?

          That seems good? I think it should default to rounding up? Apps that want to tune RAM usage down can then change the default.

          Show
          Michael McCandless added a comment - I think we need to make this (RAM vs CPU tradeoff) settable by the app, at indexing time? I think we should do that. Maybe via IWC? That seems good? I think it should default to rounding up? Apps that want to tune RAM usage down can then change the default.
          Hide
          Simon Willnauer added a comment -

          That seems good? I think it should default to rounding up? Apps that want to tune RAM usage down can then change the default.

          +1 I will create an issue

          Show
          Simon Willnauer added a comment - That seems good? I think it should default to rounding up? Apps that want to tune RAM usage down can then change the default. +1 I will create an issue
          Hide
          Simon Willnauer added a comment -

          +1 I will create an issue

          FYI - LUCENE-3509

          Show
          Simon Willnauer added a comment - +1 I will create an issue FYI - LUCENE-3509
          Hide
          Martijn van Groningen added a comment -

          one thing I noticed about the patch is that you consider 0 ords as null values which is not the case in IDV.

          I changed that in the new patch.
          I also included IDV implementation for AbstractFirstPassGroupingCollector.

          I think it is time to include these collectors into the already existing tests. Besides that we need IDV implementations for AbstractSecondPassGroupingCollector and AbstractAllGroupHeadsCollector.

          Show
          Martijn van Groningen added a comment - one thing I noticed about the patch is that you consider 0 ords as null values which is not the case in IDV. I changed that in the new patch. I also included IDV implementation for AbstractFirstPassGroupingCollector. I think it is time to include these collectors into the already existing tests. Besides that we need IDV implementations for AbstractSecondPassGroupingCollector and AbstractAllGroupHeadsCollector.
          Hide
          Martijn van Groningen added a comment -

          After a svn update (that includes changes for LUCENE-3507) the heap usage for grouping by BYTES_VAR_SORTED IDV and using disk resident values is much more stable! It stays between 5MB and 16MB and the average search time is around 580ms.

          Show
          Martijn van Groningen added a comment - After a svn update (that includes changes for LUCENE-3507 ) the heap usage for grouping by BYTES_VAR_SORTED IDV and using disk resident values is much more stable! It stays between 5MB and 16MB and the average search time is around 580ms.
          Hide
          Martijn van Groningen added a comment -

          Updated patch to work with the trunk.

          Show
          Martijn van Groningen added a comment - Updated patch to work with the trunk.
          Hide
          Martijn van Groningen added a comment -

          Updated patch. Added idv implementation for second pass grouping collector. Also the tests have been changed to randomly select an idv based implementation when possible (tests now choose randomly between term, idv and function based implementations).

          Show
          Martijn van Groningen added a comment - Updated patch. Added idv implementation for second pass grouping collector. Also the tests have been changed to randomly select an idv based implementation when possible (tests now choose randomly between term, idv and function based implementations).
          Hide
          Martijn van Groningen added a comment -

          Updated patch. Moved Simon's changes to force the use of the sorted variants to use fixed size packed ints to LUCENE-3509.

          Show
          Martijn van Groningen added a comment - Updated patch. Moved Simon's changes to force the use of the sorted variants to use fixed size packed ints to LUCENE-3509 .
          Hide
          Martijn van Groningen added a comment -

          I think this can be committed soon. The fixed size packed ints options for DV will be further improved in LUCENE-3509.

          Show
          Martijn van Groningen added a comment - I think this can be committed soon. The fixed size packed ints options for DV will be further improved in LUCENE-3509 .
          Hide
          Simon Willnauer added a comment -

          Martjin, the last patch looks ok to me. you should go ahead and commit this...

          Show
          Simon Willnauer added a comment - Martjin, the last patch looks ok to me. you should go ahead and commit this...
          Hide
          Martijn van Groningen added a comment -

          I was planning on doing this. I'm almost ready to commit it. I'm only a bit stuck on documents that don't have a value for a group field.

          The random grouping tests also add documents with a null value for the group field and an empty string for the group field. This works fine with the term based implementations, but not the DV based implementations (random test fail). Should we not use null as group value if the dv based implementations are used during the test?

          Show
          Martijn van Groningen added a comment - I was planning on doing this. I'm almost ready to commit it. I'm only a bit stuck on documents that don't have a value for a group field. The random grouping tests also add documents with a null value for the group field and an empty string for the group field. This works fine with the term based implementations, but not the DV based implementations (random test fail). Should we not use null as group value if the dv based implementations are used during the test?
          Hide
          Martijn van Groningen added a comment -

          Updated patch.

          • All tests pass now.
          • Added dv based impl for AllGroupHeadCollector.

          I will commit this soon.

          Show
          Martijn van Groningen added a comment - Updated patch. All tests pass now. Added dv based impl for AllGroupHeadCollector. I will commit this soon.
          Hide
          Martijn van Groningen added a comment -

          Committed to trunk.

          Show
          Martijn van Groningen added a comment - Committed to trunk.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development