As we introduce more places where we add concurrency (there are
currently three) there is a common pattern around checking whether there
is an executor provided, and then going sequential on the caller thread
or parallel relying on the executor.
That can be improved by internally creating a TaskExecutor that relies
on an executor that executes tasks on the caller thread, which ensures
that the task executor is never null, hence the common conditional is no
longer needed, as the concurrent path that uses the task executor would
be the default and only choice for operations that can be parallelized.
The "create github release" step was missing from the release wizard. We have forgotten about it a few times recently.
While at it, I also expanded the instructions around closing the current milestone and moved them after removing opened issues / PRs from the current milestone.
We recently made TaskExecutor public. It currently exposes two methods:
one to create tasks given a collection of callables, and one to execute
all tasks created at step 1. We can rather expose a single public method
that takes a collection of callables which internally creates the
appropriate tasks. This simplifies the API, and stops us from leaking
the internal Task abstraction which can be kept private.
Note that this is backwards compatible as we have not released yet a
version where the TaskExecutor was made public. It is marked
experimental anyways.
Add RandomAccessInput#length method to the RandomAccessInput interface. In addition it deprecates
ByteBuffersDataInput#size in favour of this new method.
Until now, LuceneTestCase#newSearcher randomly associates the returned
IndexSearcher instance with an executor that is ad-hoc created, which
gets shut down when the index reader is closed.
This has made us catch a couple of cases where we were not properly
closing readers in tests. Most recently, we have been seeing test
failures (OOM - unable to create thread) due to too many executor
instances created as part of the same test. This is to be attributed to
creating too many searcher instance, each one getting its separate
executor, which all get shutdown at the end of the entire suite. The
main offender for this is QueryUtils which creates a new searcher for
each leaf reader, and the top-level reader gets closed in the
AfterClass, hence all the executors will stay around for the entire
duration of the test suite that relies on QueryUtils.
This commit eagerly creates an executor in an additional before class
method for LuceneTestCase, and associates that with each searcher that
is supposed to get a non null executor.
Note that the executor is shutdown in the after class to ensure that
no threads leak in tests.
This has the additional advantage that it removes the need to close the
executor as part of an index reader close listener, which also requires
the reader to have an associated reader cache helper.
This implements a specialized `BlockMaxConjunctionBulkScorer`, which is really
the same as `BlockMaxConjunctionScorer`, but as a `BulkScorer` instead of a
`Scorer`. Also it doesn't support two-phase iterators in order to focus on the
common case when queries, such as term queries, do not have two-phase
iterators. If a clause has a two-phase iterator, it will keep running as a
`BlockMaxConjunctionScorer` wrapped in a `DefaultBulkScorer`.
This commit adds the possibility to read / write binary stored values using a DataInput and the number of bytes. By default the implementations will allocate those bytes in a newly created byte array and call the already existing method.
TaskExecutor is currently package private. We have scenarios where we
want to parallelize the execution and reuse it outside of its package,
hence this commit makes it public (and experimental).
Note that its constructor remains package private as it is supposed to
be created by the index searcher, and later retrieved from it via the
appropriate getter, which is also made public as part of this commit.
Co-authored-by: gf2121 <52390227+gf2121@users.noreply.github.com>
Concurrent search is currently applied once per search call, either when
search is called, or when concurrent query rewrite happens. They
generally don't happen within one another. There are situations in which
we are going to introduce parallelism in places where there could be
multiple inner levels of parallelism requested as each task could try to
parallelize further. In these cases, with certain executor
implementations, like ThreadPoolExecutor, we may deadlock as we are
waiting for all tasks to complete but they are waiting for threads to
free up to complete their execution.
This commit introduces a simple safeguard that makes sure that we only
parallelize via the executor at the top-level invokeAll call. When each
task tries to parallelize further, we just execute them directly instead
of submitting them to the executor.
Co-authored-by: Adrien Grand <jpountz@gmail.com>
When re-using the HNSW graph during segment merges, it is possible that more than the configured M*2 connections could be made per vector.
In those instances, we should allow the graph to still be read from the codec and searchable.
The default ForkJoinPool implementation uses a thread factory that removes all
permissions on threads, so we need to create our own to avoid tests failing
with FS-based directories.
Recursive graph bisection is an extremely effective algorithm to reorder doc
IDs in a way that improves both storage and query efficiency by clustering
similar documents together. It usually performs better than other techniques
that try to achieve a similar goal such as sorting the index in natural order
(e.g. by URL) or by a min-hash, though it comes at a higher index-time cost.
The [original paper](https://arxiv.org/pdf/1602.08820.pdf) is good but I found
this [reproducibility study](http://engineering.nyu.edu/~suel/papers/bp-ecir19.pdf)
to describe the algorithm in more practical ways.
`ImpactsDISI` is nice: you give it an `ImpactsEnum`, typically coming from the
`PostingsFormat` and it will automatically skip hits whose score cannot be
greater than the minimum competitive score. This is the class that yields 10x
or more speedups on top-level `TermQuery`s compared to exhaustive evaluation.
However, when nested under a disjunction or a conjunction, `ImpactsDISI`
typically adds more overhead than it enables skipping. The reason is that on a
disjunction `a OR b`, the minimum competitive score of `a` is the minimum score
for the disjunction minus the maximum score of `b`. While this sort of
propagation of minimum competitive scores down the query tree sometimes helps,
it does hurt more than it helps on average, because `ImpactsDISI` adds quite
some overhead and the per-clauses minimum scores are usually so low that they
don't actually enable skipping hits. I looked into reducing this overhead, but
a big part of it is the additional virtual call, so the only way to get rid of
this overhead is to not wrap with an `ImpactsDISI` at all.
This means that scorers need a way to know whether they are producing the
top-level score, or whether they are producing a partial score that then gets
combined into the top-level score. Term queries would then only wrap with
`ImpactsDISI` when they produce the top-level score. Note that this does not
only include top-level term queries, but also conjunctions that have a single
scoring clause (`a #b`) or combinations of a term query and one or more
prohibited clauses (`a -b`).
This PR involves the refactoring of the HNSW builder and searcher, aiming to create an abstraction for the random access and vector comparisons conducted during graph traversal.
The newly added RandomVectorScorer provides a means to directly compare ordinals, eliminating the need to expose the raw vector primitive type.
This scorer takes charge of vector retrieval and comparison during the graph's construction and search processes.
The primary purpose of this abstraction is to enable the implementation of various strategies.
For example, it opens the door to constructing the graph using the original float vectors while performing searches using their quantized int8 vector counterparts.
* Document why we need `lastPosBlockOffset`
* Let ./gradlew tidy fix the formatting
* Fix '<' with <
---------
Co-authored-by: Tony Xu <tonyx@amazon.com>
There are a few places where tests don't close index readers. This has
not caused problems so far, but it becomes an issue when the reader gets
an executor, because its shutdown happens as a closing listener of the
reader. This has become more evident since we now offload sequential
execution to the executor. If there's an executor, but it's never used,
no threads are created, and no threads are leaked. If we do use the
executor, and the reader is not closed, the test leaks threads.
When an executor is set to the IndexSearcher, we should try and offload
most of the computation to such executor. Ideally, the caller thread
would only do light coordination work, and the executor is responsible
for the heavier workload. If we don't offload sequential execution to
the executor, it becomes very difficult to make any distinction about
the type of workload performed on the two sides.
Closes#12498
When performing concurrent search, we may get an execution exception
from one or more slices. In that case, we'd like to rethrow the cause of
the execution exception, which we do by wrapping it into a new runtime
exception. Instead, we can rethrow runtime exceptions as-is, and the
same is true for io exceptions. Any other exception is still wrapped
into a new runtime exception. This unifies the exceptions that get
thrown between sequential codepath (when no executor is provided) and
concurrent codepath (when an executor is provided).
Adrien Grand
Luca Cavanna
elliotzlin
Uwe Schindler
gf2121
Patrick Zhai
Christine Poerschke
Ignacio Vera
Shubham Chaudhary
Tyler Bertrand
Tony-X
Benjamin Trent
Dawid Weiss
Egor Potemkin
Greg Miller
Gokul Manoj
Jim Ferenczi
Michael McCandless