Enable nightly tests comparing classic facets with the new during-col… - #518
Enable nightly tests comparing classic facets with the new during-col…#518epotyom wants to merge 5 commits into
Conversation
07a9f22 to
07a8d1b
Compare
…lection facets Changelist: 1. Add support for `TASK+SUBTASK` format, for nightly tests all subtasks are added to the same graph 2. Separate nightly search and facet tasks files and execution - it is required as facets require slightly different configuration to be able to compare old/new API, see details in mikemccand#325 (comment) 3. Allow missing task in candidate if benchUtil#simpleReport - required to remove facet tasks from search tasks in [2] 4. Allow missing previous results 5. Fix script name typo in README.md (not related to nightlies) # Conflicts: # src/python/nightlyBench.py # Conflicts: # src/python/nightlyBench.py
07a8d1b to
a9331c0
Compare
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
Results from my machine: reports.nightly.tar.gz You can look at the links under the Faceting header to see graphs that show classic/sandbox facet API side-by-side results, for example: |
Ha, love that branch name! Sorry... |
Holy smokes, the during-collection kicks butt! 9.2 QPS vs 3.0 QPS!? Do we validate that they produce the same results? Are they always expected to? |
mikemccand
left a comment
There was a problem hiding this comment.
This is awesome -- I can't wait to run in nightly beast3 benchy.
I'm confused why you need a whole separate JVM just for the facet tasks...
| # When we add a whole new task (e.g. VectorSearch), just skip the comparison for the first nightly run | ||
| # since baseline will not have this task yet: | ||
| continue | ||
| if currentCat not in cmpLatencyMetrics: |
There was a problem hiding this comment.
Whoa, this shows that you really got a nightly benchy up and running! Because this is a back-compat layer for the first time nightly benchy runs after this is merged... phew. Thank you for the amazing attention to detail on this.
| NIGHTLY_DIR, | ||
| index=index, | ||
| # vectorDict=(constants.VECTORS_WORD_TOK_FILE, constants.VECTORS_WORD_VEC_FILE, constants.VECTORS_DIMENSIONS), | ||
| vectorFileName=constants.VECTORS_QUERY_FILE, |
There was a problem hiding this comment.
Hmm do we need vectors for this?
Oh, I see, we want this competitor to reuse the same deterministic search index, so maybe we have to match all of its config or so?
There was a problem hiding this comment.
Yes, exactly, just want to reuse the index
| # because we want to compare post-collection and during-collection facet performance. | ||
| # Post-collection facets usually utilize the main thread, and during-collection | ||
| # facets use the searcher thread. As a result, we might get arbitrary results | ||
| # if we shuffle tasks instead of grouping them by category. |
There was a problem hiding this comment.
I don't quite understand -- why can't they just be additional tasks in the wikinightly.tasks and we use the one JVM to go through all tasks including "during collection facets"? I guess we'd need to enhance that baby tasks language to express/recognize this alternative facets impl?
There was a problem hiding this comment.
Replied below in #518 (comment). As for the tasks format, support for that was added earlier in #325.
| resPrev.append(prevFName) | ||
|
|
||
| if len(resultsPrev) == 0 and DEBUG: | ||
| # TODO: It used to require previous results in non-DEBUG mode, but that doesn't work if you run nightlies first time ever on your machine, |
There was a problem hiding this comment.
Wow, awesome that you tested to this point and uncovered this bootstrapping problem. I've since relaxed this (removed the and DEBUG) from main I think? I was somehow hitting the same bootstrapping problem when recovering from the horrible new Linux CPU governor can't talk to old BIOS nightly benchy regression issue!
I think -reset on the command-line should let you bootstrap, but +1 to just make it all resilient to not finding past results ... hmm, that is a bit lenient, and might hide other bugs? Not sure that matters.
There was a problem hiding this comment.
Oh, looks like I missed that -reset fixes it. The first run requires -reset anyway, as otherwise it fails at some point later, so I think keeping this behind -reset works.
| comp = c | ||
| comp.tasksFile = f"{constants.BENCH_BASE_DIR}/tasks/wikinightly.tasks" | ||
| comp.printHeap = True | ||
| nightly_competitor.tasksFile = f"{constants.BENCH_BASE_DIR}/tasks/wikinightly.tasks" |
There was a problem hiding this comment.
Thank you for the naming improvements!
| # For Search/Facets tasks headers/number of columns can be dynamic as it depends on number of subcategories. | ||
| # If there are no subcategories, use "QPS" by default | ||
| # The format for task with subcategories is "CATEGORY+SUBCATEGORY: ..." | ||
| searchChartHeaders = {} |
There was a problem hiding this comment.
Hmm, maybe add an example in the comment here? What exactly is a subcategory?
There was a problem hiding this comment.
Basically, subcategory is just a way to group graphs in the nightly test results. I figured it might be useful for something else later, so I didn't want to make it facet-specific. Hope that works!
Adding a comment in the next revision.
| if isinstance(cat, bytes): | ||
| # TODO: why does this happen!? | ||
| cat = str(cat, "utf-8") | ||
| # Show all subcategory graphs together |
There was a problem hiding this comment.
Ahh is subcategory for this PR/case which facets impl?
There was a problem hiding this comment.
Yes, basically for tasks
OrHighMedDayTaxoFacets+postCollection
OrHighMedDayTaxoFacets+duringCollection
we create a single graph for OrHighMedDayTaxoFacets with two lines - one for postCollection QPS and one for duringCollection QPS.
| OrHighMedDayTaxoFacets+postCollection: not opening +post_collection_facets:DayOfYear.taxonomy # freq=1713264 freq=84576 | ||
| OrHighMedDayTaxoFacets+postCollection: which pre +post_collection_facets:DayOfYear.taxonomy # freq=1963428 freq=90549 | ||
|
|
||
| OrHighMedDayTaxoFacets+duringCollection: 4 steve +during_collection_facets:DayOfYear.taxonomy # freq=986452 freq=84364 |
There was a problem hiding this comment.
Oh ok so the baby tasks language is already extended to understand during_collection_facets, yay!
Oh actually is this a tricky comparison because during-collection facets can use threads but post-collection facets does not (in We somehow need a cleaner way for luceneutil to normalize for / communicate this in general. The wall-clock elapsed time for a task is really important since it shows how effectively the task's execution can soak up available CPU concurrency, and what your QPS would be if you had "effectively infinite" CPU cores relative to your query traffic (you are "mostly idle-ish"). The total CPU ticks (summed across all threads that worked on that task) is also important, since that dictates how much CPU oomph The existing nightly charts (I think?) are "effective QPS if you had infinite CPU cores" (1.0 / wall-clock-latency). Hmm, but we recently changed the nightly benchy to exercise Lucene's intra-query concurrency by switching to only one query in flight at a time and N CPU cores available for concurrent execution of that one query. OK I see, we still have #275 open, tricky. From that issue, and looking at Anyway, we must make progress on #275 ... how to best reflect these slippery measures (in charts, and in console output). |
We see similar QPS wins in #325, where we do compare results. However, for nightlies we don't compare during/post facet results since they are essentially different tasks in the task file. I need to think about the best way to enable comparing results across different tasks - let me know if you have any ideas!
Correct, and not just in luceneutil, classic facets are generally sequential, with few exceptions. That said, I do expect some QPS wins since during-collection facets skip creating docIdSets for matches, so they should be somewhat faster. At the same time, there is some overhead from managing concurrency, so the final results likely depend on the task. Some of the luceneutil result improvement (and regression for the
This is actually an attempt to make the single-thread vs. concurrent comparison fairer. Details are in #325 (comment), but the tl;dr is that we want to group tasks by category so that each category can maximize CPU usage, whether it uses intra- or inter+intra-query concurrency. Oh, but I just noticed that unlike |
|
I'm rerunning the test with The plan for the facet tests is to set all three parameters to 8 so that the benchmark tries to utilize 100% of 8 CPUs, whether through inter- or intra-query concurrency. Grouping tasks by category ensures that tasks don't steal CPU time from one another. What do you think, @mikemccand? |
…t when to use subcategories
Use the param for nightly and runFacets.py tests to make comparison between inter and intra concurrent queries (during collection facets) and inter concurrent only (post collection facets) more fair. mikemccand#518
|
Made these changes based on this comment: #518 (comment) Rerunning the This looks expected — all the All tasks that involve matching + facets perform better with during-collection facets. |
|
Nightly results match local test results above; see screenshots below. They highlight opportunities to improve during-collection facets in certain scenarios—in particular, the match-all-documents case ( What is in scope:
|
I originally had |
Use the param for nightly and runFacets.py tests to make comparison between inter and intra concurrent queries (during collection facets) and inter concurrent only (post collection facets) more fair. mikemccand#518
+1
+1 -- right now in nightly I think we only test the intra-query concurrency ( |
|
Confirmed that I added implementing I also confirmed that |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
Phew, this almost fell past the event horizon of my TODO list ... I'll try to page it all back in and move forward. This has some big changes to the nightly benchy right? Like separate JVM for each task or so? But mixing tasks across threads within one JVM was intentional because it forced hotspot to pay attention to N different things at once so it could not over-train/optimize to singleton task. But then we changed nightly benchy at some point to do only intra-query concurrency, so we lost that hotspot stretching. Thank you for all the exciting work here @epotyom -- it's awesome to have this faster facets engine, and it really is important we track it in nightly benchy. |
|
Thanks for reviewing @mikemccand!
Right now it only starts a separate JVM for facet tasks - not a separate JVM per category. Within that JVM, facet tests are configured to:
Without this, comparing classic facets (mostly intra-query concurrent) to during-collection facets (both inter- and intra-query concurrent) is unfair - task performance depends heavily on what other tasks are running in parallel and which thread pool they compete for.
Good point, the changes above do break this. The intent is to give the same resources to each category and compare them at their best. But since we are not creating a separate JVM per category, results can be skewed: the warmup phase in SearchPerfTest optimizes for the first category's code paths, and the second category starts cold. It may not be as bad as it sounds given the high I see two options:
What do you think? |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
I've been thinking about #275 , in particular about running tasks from different categories concurrently and measuring fair performance for each category separately. I'm not sure it is truly feasible. Even if we can count CPU utilization per category, I'm not sure we can fairly isolate IO, memory bandwidth, etc. I think that the underlying issue is that we want macro benchmark conditions (realistic concurrent load) but per-category micro benchmark results. I'm not sure we can have both at once. Proposal: split into separate macro and micro benchmark runs, in separate JVMs. Macro benchmarks: interleave tasks across categories as today, but report a single aggregate QPS across all categories. Interleaving is the right approach here - it prevents hotspot from over-optimizing for one code path - but extracting fair per-category results from a concurrent mixed run isn't feasible, so we don't try. Micro benchmarks: run one category at a time so each has full access to resources and results are comparable across categories and stable day-over-day. Concretely:
WDYT @mikemccand ? |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |









…lection facets
Changelist:
CATEGORY+SUBCATEGORYformat, for nightly tests all subcategories are added to the same graphTested locally on top of changes from this branch to enable nightly benches main...epotyom:luceneutil:fix_nightly_start_from_scratch