Enable comparing main facets module agains sandbox facets implementation #325

epotyom · 2025-01-07T23:58:43Z

Auxiliary changes:

SearchTask#getFacetResultsMsec now includes time to run #search, because in the sandbox module we can't measure search and facet compute times separately. But it might be the right thing to do anyway, as otherwise we don't account for time spent to build doc ID sets in FacetsCollector?
Enable attaching a context to Task, to be able to use different implementations for the same task and Lucene code in baseline/candidate
Fix facets result overlap check: in python SearchTask's equals and hash methods compare facets requests, not results
Added facetsWikimediumAll config that contains all taxonomy facets tasks from wikimediumall

Command to run:

python src/python/localrunFacets.py -source facetsWikimediumAll

Results on my laptop

Report after iter 19:

                            TaskQPS classic_facets      StdDevQPS sandbox_facets      StdDev                Pct diff p-value
     BrowseRandomLabelTaxoFacets        5.99      (6.0%)        2.64      (1.6%)  -55.9% ( -59% -  -51%) 0.000
                        PKLookup      297.27      (5.8%)      251.45      (4.9%)  -15.4% ( -24% -   -5%) 0.000
         AndHighMedDayTaxoFacets      154.40      (3.2%)      153.00     (14.8%)   -0.9% ( -18% -   17%) 0.788
        AndHighHighDayTaxoFacets       13.13      (3.4%)       13.04     (11.7%)   -0.7% ( -15% -   14%) 0.792
          OrHighMedDayTaxoFacets       16.27      (5.5%)       19.52     (20.0%)   20.0% (  -5% -   48%) 0.000
       BrowseDayOfYearTaxoFacets        6.52      (7.7%)        9.30     (12.9%)   42.8% (  20% -   68%) 0.000
            BrowseDateTaxoFacets        6.48      (7.6%)        9.68     (13.9%)   49.3% (  25% -   76%) 0.000
           BrowseMonthTaxoFacets        6.21      (6.9%)        9.38     (15.1%)   51.0% (  27% -   78%) 0.000
            MedTermDayTaxoFacets       38.03      (3.9%)       71.68     (25.3%)   88.5% (  57% -  122%) 0.000

I'll look into why there is regression for BrowseRandomLabelTaxoFacets and PKLookup

epotyom · 2025-01-09T16:25:44Z

There is BrowseRandomLabelTaxoFacets regression because RandomLabel.taxonomy field is the one that uses most of the sidecar taxonomy index, and it seems to be the only field for which dense counting (in array) pays off; total array (and taxon index) size is 1559911, and when counting is done, only 3422 (0.2%) elements are still zeros.

We can think about implementing dense counting for the sandbox facet module, but it looks like a rare use case to me - not only we need a field with large number of unique values, but it also needs to be a MatchAllDocsQuery.

In any case, seems like the result is expected. Looking into PKLookup regression now.

stefanvodita

SearchTask#getFacetResultsMsec now includes time to run #search

Will that change results we were already reporting? Just good to know.

stefanvodita · 2025-02-04T14:24:35Z

src/main/perf/SearchTask.java

-import org.apache.lucene.facet.Facets;
-import org.apache.lucene.facet.FacetsCollector;
-import org.apache.lucene.facet.FacetsCollectorManager;
+import org.apache.lucene.facet.*;


We probably want to avoid the wildcard import.
(Same below)

Oh I think my Intellij folded it, I'll change

stefanvodita · 2025-02-04T14:26:25Z

src/main/perf/SearchTask.java

+          // TODO: support sort, filter too!!
+          // TODO: support other facet methods
+          List<TaskParser.TaskBuilder.FacetTask> classicFacetRequests = new ArrayList<>();
+          List<TaskParser.TaskBuilder.FacetTask> sandboxFacetRequests = new ArrayList<>();


We're starting to spread this temporary name. It would have been great to converge on apache/lucene#13965 before, but I don't know that we want to wait now.

I say @epotyom and @Shradha26 should simply pick a name, now, and run with it, here :) Surprise us!

The way I'm thinking about it is that new classes can be moved to the existing facets module once we believe they are mature enough. I don't think these classes belong in an independent module, they depend on a lot of things from the facets module, e.g. indexing time functionality or drill sideways. I promise to clean up and get rid of all mentions of "sandbox" once we do it, or, if decided otherwise, once we create a separate module for it. What do you think?

It's a good point that the new classes may become part of the facets module.
In this PR though, we're using "sanbox" as a contrast to "classic". Can we name it something generic, such as "matchTime", or are we not that detached from the particular implementation?

You have a point. How about postCollectionFacets for classic VS directCollectionFacets or directFacets for sandbox? Or docIdSetFacets VS directFacets?

I like postCollectionFacets. Can we pair it with duringCollectionFacets, withCollectionFacets, or collectionFacets?

stefanvodita · 2025-02-04T14:36:56Z

src/main/perf/TestContext.java

+    public enum FacetMode {
+        UNDEFINED,
+        CLASSIC,
+        SANDBOX,  // TODO: better names?


stefanvodita · 2025-02-04T14:38:34Z

src/python/benchUtil.py

@@ -1193,7 +1195,9 @@ def runSimpleSearchBench(self, iter, id, c,
    command += [f'-XX:StartFlightRecording=dumponexit=true,maxsize=250M,settings={constants.BENCH_BASE_DIR}/src/python/profiling.jfc' +
                f',filename={constants.LOGS_DIR}/bench-search-{id}-{c.name}-{iter}.jfr',
                '-XX:+UnlockDiagnosticVMOptions',
-                '-XX:+DebugNonSafepoints']
+                '-XX:+DebugNonSafepoints',
+                # '-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=localhost:7891'


Did you leave this in accidentally?

It was intentional, it makes it easy to enable remote debug - just uncomment this line. Maybe I should add a comment. Do you think it's better to remove?

Personally, I don't mind leaving it in if you found it helpful for quick debugging.

stefanvodita · 2025-02-04T14:42:20Z

src/python/localrunFacets.py

+import competition
+import os
+
+# Script to compare performance of sandbox and main facets modules


Should we mention this file somewhere visible, maybe in the README?

Good idea, will do in the next revision.

mikemccand · 2025-02-04T15:54:13Z

src/main/perf/TestContext.java

@@ -0,0 +1,40 @@
+package perf;


Needs copyright header?

mikemccand · 2025-02-08T14:13:46Z

otherwise we don't account for time spent to build doc ID sets in FacetsCollector?

+1 -- good catch!

epotyom · 2025-02-10T22:26:45Z

Thank you for reviewing @stefanvodita !

SearchTask#getFacetResultsMsec now includes time to run #search

Will that change results we were already reporting? Just good to know.

Yeah, but TBH I can't find where we report getFacetResultsMsec. We print it to log, and then benchUtil reads it. Not sure what happens to it next. Maybe it is used in a different repo?

I believe that QPS metrics reported by localrun.py are not going to change.

Command to run: python src/python/localrunFacets.py -source facetsWikimediumAll Auxiliary changes: - Enable attaching context to a Task, to be able to use different implementations for the same task and Lucene code in baseline/candidate - Fix facets result overlap check: in python SearchTask's equals and hash methods to compare facets request, not results - Added facetsWikimediumAll config that contains all taxonomy facets tasks from wikimediumall

stefanvodita

I left a few more small comments. Thank you for making the changes, great to see this change coming along!

stefanvodita · 2025-02-11T22:39:24Z

README.md

+
+There are currently two facets implementations - one that first collects document IDs and then computes facets in a separate phase, and a new implementation that computes facets during collection.
+
+To compare performance for the two implementations run


Should we add details about what exactly gets compared? For example, you had a comment about only taxonomy facets being compared I think.

I agree, added.

stefanvodita · 2025-02-11T22:40:24Z

src/main/perf/SearchTask.java

+            //       for MatchAllDocsQuery to make collection for all docs in the index faster?
+            Map<String, CountFacetRecorder> indexFieldToRecorder = new HashMap<>();
+            List<CollectorManager<? extends Collector, ?>> collectorManagers = new ArrayList<>();
+            // First collector manager in the list is to collect hits, but not for if MatchAllDocsQuery


Can we state this more clearly?

stefanvodita · 2025-02-11T22:44:08Z

src/main/perf/SearchTask.java

+                if (request.dimension().startsWith("range:")) {
+                  throw new AssertionError("fix me!");
+                } else if (request.dimension().endsWith(".taxonomy")) {
+                  //if (true) throw new RuntimeException("fix me! " + request.dimension() + "; " + state.facetsConfig.getDimConfig(request.dimension()).indexFieldName);


Forgotten commented code?

Removed, sorry for the mess!

stefanvodita reviewed Feb 4, 2025

View reviewed changes

mikemccand reviewed Feb 4, 2025

View reviewed changes

houserjohn mentioned this pull request Feb 6, 2025

Add benchmark coverage for dynamic numeric range faceting #311

Open

epotyom and others added 5 commits February 10, 2025 23:04

Minor follow up changes

27908eb

localrunFacets comment updates

8f648d9

Addressed PR comments

52bad48

Addresing more PR comments

3c46276

epotyom force-pushed the facets_compare branch from c446471 to 3c46276 Compare February 11, 2025 22:25

stefanvodita approved these changes Feb 11, 2025

View reviewed changes

More comments are addressed

dac76d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable comparing main facets module agains sandbox facets implementation #325

Enable comparing main facets module agains sandbox facets implementation #325

epotyom commented Jan 7, 2025

epotyom commented Jan 9, 2025

stefanvodita left a comment

stefanvodita Feb 4, 2025

epotyom Feb 10, 2025

stefanvodita Feb 4, 2025

mikemccand Feb 8, 2025 •

edited

Loading

epotyom Feb 10, 2025

stefanvodita Feb 10, 2025

epotyom Feb 10, 2025

stefanvodita Feb 11, 2025

stefanvodita Feb 4, 2025

stefanvodita Feb 4, 2025

epotyom Feb 10, 2025

stefanvodita Feb 11, 2025

stefanvodita Feb 4, 2025

epotyom Feb 10, 2025

mikemccand Feb 4, 2025

epotyom Feb 11, 2025

mikemccand commented Feb 8, 2025

epotyom commented Feb 10, 2025

stefanvodita left a comment

stefanvodita Feb 11, 2025

epotyom Feb 11, 2025

stefanvodita Feb 11, 2025

epotyom Feb 11, 2025

stefanvodita Feb 11, 2025

epotyom Feb 11, 2025


		There are currently two facets implementations - one that first collects document IDs and then computes facets in a separate phase, and a new implementation that computes facets during collection.

		To compare performance for the two implementations run

Enable comparing main facets module agains sandbox facets implementation #325

Are you sure you want to change the base?

Enable comparing main facets module agains sandbox facets implementation #325

Conversation

epotyom commented Jan 7, 2025

epotyom commented Jan 9, 2025

stefanvodita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemccand Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemccand commented Feb 8, 2025

epotyom commented Feb 10, 2025

stefanvodita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemccand Feb 8, 2025 •

edited

Loading