When starting up, we can have a large number of views, each
with some number of fragments, and by default these were being
opened sequentially. There's no real benefit to that; they're
all nicely independent from each other and don't need much
locking, so we implement a trivial semaphore and launch the
operations asynchronously. We also combine them into
errgroups.
Similarly, we do this for fields and views, capping the number
of fields (or views) opened in parallel to avoid hitting a
system-wide limit on threads created (oops). Note that the
limits are shared, not multiplicative; we cap this fairly
arbitrarily at 8 fields being opened, and 16 views being opened,
at a time, but NumCPU*2 fragments being opened by those views.
This dramatically increases CPU load during startup, but doesn't
seem to significantly increase total CPU time, it just scales
much better on machines with lots of cores.
The new roaring iterator used for the remap and importroaring
things could also be used for unmarshalling roaring streams,
and it's a slightly simpler design that doesn't need two passes
through the data. This patch cleans that up a bit, makes it work
better with ops logs, and uses that instead. It appears to
noticably but not immensely reduce the time imports take, but it
also gets us back down to one thing parsing roaring formats.
There are a couple of subtle changes to errors we were testing
for in various tests, and one of the fuzz tests goes away because
it was actually itself an erroneous error message -- it was reporting
the header of a roaring file as an invalid op because the op log
reader was running on the header for roaring files with zero
containers. Oops.
we are experiencing issues with CI where it fails with race: limit on
8128 simultaneously alive goroutines is exceeded, dying
this, despite the fact that closing the executor should clean up all
worker goroutines. Apparently in CircleCI runtime.NumCPU() reports 36,
so the goroutines added up quickly.
the size of the work chan probably doesn't matter... there is some
discussion of this on the associated PR
https://github.com/pilosa/pilosa/pull/2034
may test with an unbuffered channel as well.
Closing the executor avoids leaking goroutines which seems to be an
issue while running the test suite.
Pilosa previously spawned a goroutine for each remote node that a
query needed to be forwarded to, and then forwarded a single request
containing all the shards that the query should operate on. It then
spawned a goroutine *per local shard* to process the query
locally. This was fine if there weren't too many shards, or too many
queries coming in concurrently, but we found that it created issues
when there were 100s or 1000s of shards per node, and dozens of
queries arriving concurrently.
Specifically, the memberlist "hiccup" issue is highly correlated with
many goroutine scenarios, and after applying this patch, memberlist
complaints in the logs were much decreased, and nodeLeave events under
concurrent query load almost entirely eliminated.
This patch creates a fixed size pool of goroutines to do local shard
processing, and passes work to them through a channel, one job per
query per shard. Handling of remote requests (forwarding queries) is
unchanged.
We set the pool size to NumCPU()+8 somewhat arbitrarily, but this
seemed to work pretty well in our testing on 32 core machines. It's a
pretty big improvement over launching a goroutine per shard per query
which is what we were doing previously, so we can tune it more later
if necessary.
Have found some potential performance or stability issues associated
with lots of mutex blocking in getting a parent span's context. Want
the ability to totally disable tracing to help debugging.
Annoyingly, this is actually the only place we can make
a read-only lock, because the row() call might write to
the row cache, so it needs the write lock. We might be
able to fix that later, though.
Addressing various lint.
incrementOpN no longer returns errors, because it no longer waits for
the snapshot, so checking those errors is unnecessary.
Several fields in a common embedded structure were "unused" according
to a naive checker.
Other tiny style things, and one actual unchecked error. Yay linters!
If you don't hold the fragment lock when computing rows, it's
pretty reasonable for other stuff to be able to modify it -- which
could invalidate or race the enumeration.
Some calls to f.rows were being made with the lock held, others
weren't, so we introduce `f.unprotectedRows` which has the obvious
semantics. (Without which this looked great except that several
of the tests deadlocked.)
The Pilosa roaring format uses two bytes of its
header, next to the magic number, for a version. The
official roaring format uses them for a container
count, if and only if it's the version of the format
that uses run-length containers.
But if it is, it really does need those bits. Also,
since we never use the official format in our internals
or snapshots, we don't have any reason to support
reading flag bits in it, since the flag bits are used
only for internals of fragments and snapshots. So
we revert the change to support flags with official
roaring bitmaps.
A couple of the fuzz tests happened to rely on this,
and we may find more issues with more fuzzing.
