We only have 4 bytes for offsets, but what if a file is
over 4GB? Someone came to us with a file with 265 *million* containers,
in a single fragment, which means that over 3GB of their 4.7GB file
is actually just the container headers alone. But we can't easily make
the offsets larger, or change the file format.
So we don't. We just track how many 4GB hunks of the file we've
been through and bump that every time the 32-bit offset wraps. And this
appears to... just work.
This is fixed for both the roaring iterator and the old unmarshalBinary
logic. The logic to handle this will work on 32-bit hosts in the sense
that it will correctly error out for excessively large file sizes or
container counts, but it doesn't actually handle the large files since
it can't.
In addition to adding some tests, this commit moves the
`GenerateUint64Slice()` helper function into a new `generator` package
so that it can be used in both internal and non-internal tests.
We avoid using bitmapContains so often because that turns out to be expensive.
Also, if we produce more than runMaxSize runs, we're going to convert to
a bitmap container (or possibly an array container if there were over
2048 items, but they're all singletons), and we can streamline that by just
converting the source to bitmap and returning differenceBitmapBitmap, which
is faster in this case.
This appears to overall take about half as long in the workload I was
looking at.
UpdateEvery can change every key, and I think it strongly suggests no
reasonable expectation of repeated access to a previously-accessed key,
but also it can change the containers and replace them.
We were avoiding caching mapped containers in some but not all cases,
and that was causing segfaults. But really, the *problem* is that
the remap operation wasn't clearing (or updating) the cache. Cleaning
that up allows us to take advantage of the caching performance advantage
even when working with read-only/mapped bitmaps.
The only way to hit this:
* Have mmapped containers to begin with.
* Do reads so those containers get frozen.
* Access, either reading or writing, a specific container with key K.
* Snapshot, so the bitmap gets its containers replaced.
* Remember, they have to be frozen -- if they aren't frozen,
we'll update the containers in place.
* Now have GC run so it actually unmaps the data.
* Now try to write to the container with key K *before reading or
writing any other key*. You have to get through the whole snapshot
and GC process without any other reads or writes.
* You get the cached value. You try to use it. You explode.
The sliceContainers code was also setting lastKey to 0 in some cases,
but also setting lastContainer to nil, so this wouldn't have caused
problems, but just to be careful, I've standardized on ^uint64(0)
for everything.
differenceInPlace wasn't checking for nil containers, which are
theoretically valid empty containers. Also added a couple of other
N==0 checks to streamline the higher-level operation.
- Remove YAML magic
- Remove a lot of duplication
- Update linter
- Use parameterized jobs and matrix build
- Update Docker Hub CD to produce versioned and "latest" images
- Add custom shard width test to workflow
When the interval is a proper superset of the range with start equal to
interval start, the range must be considered a superset or it will be
completly ignored (since it neither a subset nor it overlaps)
Co-authored-by: Pierre Fersing <pierre.fersing@bleemeo.com>
Two changes:
1. Don't write batch/roaring adds or removes when N is 0, because
a write of no bits is not a meaningful write.
2. When unmarshalling roaring things, if a roaring bitmap didn't
change many bits, treat it as having changed at least 1 bit per 8 bytes,
so an 8KB hunk of roaring data counts as 1K changes, which will
nudge us towards snapshotting. This should keep us from having
Large Files show up so much.
This was particularly noticeable on the existence field, which
tends to a steady state of "completely full" very quickly in a lot
of cases.
In the old unmarshal code, the decision to mark a thing as mapped (always
yes) happens separately from setting the mapping. What if this could ever
somehow possibly go wrong? Let's sanity-check that to be extra careful.
We might have a problem with a stale mmap, and to try to narrow it down
a bit, we add some sanity-checking features and panic recovery to the
generation Transaction code.
This is pretty experimental.
So in some cases, when we do a query, the results of one
part of the query are innately shared-across-nodes; for
instance, a hypothetical Distinct query. More generally,
we allow cross-index queries; calls can have "index=foo"
in them.
This patch lets us handle that without duplicating that
query all over. Before we actually start doing the
separate calls, we run the query once from the coordinating
node, then patch the results in, and send relevant subsets
over to each client, etcetera. Also provides slightly
friendlier (and I hope faster) support for converting
bitmaps to/from sets of rows.
We also add an extension interface, and some fancy stuff
to let us define new calls, which use this. They're sort
of tied together because the first extension I wanted to
implement needed precomputed calls. The extension API
lets us create extensions using `pkg/plugin` (with all its
associated limitations, unfortunately), then query them
at load time for functionality.
This also implies some revamping of the argument
validation for PQL, like verifying that functions exist
and knowing things about their argument types.
So basically this is an overly intrusive patch, and would
be better as separate patches, but they're hard to detangle.
add trivial execution-time profiling
What if you could ?profile=true on a query and get some
numbers back? That'd be really cool.
We already have tracing/spans, but right now, those only generate
any data if you have something set up for them to trace to. Add a
fancy wrapper that lets us generate our own tracing data, and dump
it into the request response, if ?profile=true.
add a sample extension, add missing features to extension interface
Implement a naive probabilistic filter extension as an example of
what an extension looks like. In the process, discover multiple
omissions in the bitmap API. Well, I did *say* it was experimental.
This code represents an attempt at providing reliable tracking
of whether any bitmaps still in use have access to a given block
of mmapped data, allowing us to unmap the data when nothing is using
it anymore.
The basic approach is as follows: Each mmap is associated with
a new object, called a "generation". A generation reflects
a particular instance of a given file being mapped. When a
bitmap is built from an mmapped data source, the bitmap is
given a pointer to the generation as its Source. When bitmap
operations combine containers from other bitmaps, they
produce new bitmaps that are tagged with the combined set of
sources.
When we snapshot a file, or for some other reason wish to remap
it, the corresponding bitmap has all its containers updated to
use the new storage, and the bitmap's source is changed. However,
previously-handed-out containers might still have references to the
old storage. Those containers would be in bitmaps with the old
source.
After a bunch of study of trying to reference-count and track
this, I realized: We don't actually need to do that, because we
already have something suitable for determining whether anything
can reach a given object. It's the garbage collector.
So we set a finalizer on the generation object, which handles
unmapping. There's additional sanity-checks here to confirm things
like "we thought this generation should be expiring", and we
track timestamps. We could also have things check whether a
given bitmap's source was marked as obsolete "a while ago", but
that isn't implemented yet.
There's a debug version of this which tracks finalization, creation,
and ending timestamps, and has a call to provide diagnostics for
this. Identical generation IDs get separated out with random
suffixes in this case -- there's sometimes a second or third
instance of the same name due to a holder closing and reopening,
but this basically only happens in testing.
Note that generations are still used even when there's no mmapping,
but unless debugging is turned on, they shouldn't propagate much --
we don't consider a generation to be the source of a bitmap unless
the bitmap actually mapped things from that generation's mmapped
storage, or debugging is on.
There's a couple of other, possibly more subtle, changes and
bug fixes that got caught by the testing on this:
* If a fragment is partially opened and then opening some later
part fails, we close the earlier parts before returning the
error so we aren't leaving it partially open.
* Several operations on segments which were requesting that a
frozen copy of a bitmap be created are now actually *replacing*
their bitmap with the frozen bitmap, rather than discarding it.
* intersectRunRun, if it decides to create an array or bitmap,
will yield that container instead of discarding it.
And why all of this? Why, so we can actually implement the thing
where when a fragment has a valid roaring bitmap, but the ops log
is corrupt, we can truncate the corrupt part of the ops log and
reopen it. Which I did.
When the generationdebug build tag is in use, every generation
has a finalizer all the time. When it's not, they only get finalizers
when we expect them to be done -- say, when closing a fragment.
This is because finalizers appear to be possibly-expensive.
There's some logical cleanup to openStorage here, dividing part
of its work into applyStorage and importStorage, which have a common
case for handling "there's no data in this file".
Which is to say don't actually implement it, because openStorage
is too messy right now, but this is the rest of the framework,
and now I'm going to digress into fixing openStorage.
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.
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!
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.
I didn't think of this, because we don't use it much in the
client. This is a bit hairy because really official roaring
is two fairly different formats, one with runs and one without.
As the size of a fragment grows, the cost of snapshots
increases; with a large fragment getting a lot of large writes,
every write will trigger a snapshot, while any other writes have
to wait for that snapshot before they, too, can trigger a snapshot.
To address this, we introduce a background queue of snapshots.
In general, operations which were omitting their ops log writes
and just snapshotting no longer do; they emit an ops log. This does
mean that, in some cases, the ops log is written and then a snapshot
takes place essentially immediately, which costs us some performance.
However, that only actually happens under very light load; under
heavier load, there's generally going to be multiple writes coalesced
into each snapshot, and the ops log writes for them will be much
cheaper than a full snapshot.
When we do a snapshot, we may end up with containers which are
mmapped to the old file, and containers which have allocated storage
identical to the contents of the new file. It would be nicer if they
were mapped to it. But unmarshalling the entire file is expensive.
Instead, we remap it. (Or, if we couldn't mmap it, just make sure
the old stuff is no longer using the old storage space before we
munmap it.)
The new op type code changed the failure mode for
one of the fuzz test issues -- and the fuzz test revealed a
bug in the code. Fixed the code, updated the test to expect
the newer, better, message.
Also fixed capitalization on the old message.
We add a new ops log type(pair), AddRoaring and RemoveRoaring,
which set and clear the bits from a provided roaring bitmap.
This also compels us to consider additional sanity checking
during tests.
