The anti-entropy feature has never actually worked. We've been
talking about removing it or replacing it for ages, but haven't
had a concrete motivation.
But the anti-entropy interface is the sole user of several components
of the Tx interface, and now that we're trying to replace that
interface, being able to drop those components has some appeal, so
let's remove the one thing that used them, in the hopes that this
will simplify life.
This also lets us drop ForEach and ForEachRange, which were
barely used at all. The one surviving usage (CSV export) can be
handled by using the container iterator we already have, and
making ContainerCallback exported so we can use it to just call
things for every bit.
(cherry picked from commit fff9ddc1f5)
* removes unused filesize function
* removes ioutil usage
* updates ioutil.ReadAll to io.ReadAll
* updates ioutil.TempFile to os.CreateTemp
* updates ioutil.TempDir to os.MkdirTemp
* updates ioutil.ReadAll to os.ReadAll
* update ioutil.WriteFile to os.WriteFile
* updates ioutil.Discard to io.Discard
* updates ioutil.ReadDir to os.ReadDir where applicable
* removes unused code in idk
* creates type to use for context value keys
* replaces assert.Nil with assert.NoError for error checks
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)
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.
This patch replaces a lot of circumstances in which containers
were being copied with circumstances in which they are shared,
using copy-on-write semantics.
To achieve this, we emulate somewhat the design of go's
native `append` function. Operations on a container may optionally
yield a new container. A container can be marked "frozen",
after which no operation should ever write to it in any way;
that applies both to the container itself and the backing store
it refers to, if any. So for instance, instead of:
c.arrayToBitmap()
we now write:
c = c.arrayToBitmap()
Operations which need to modify a container in any way
need to be able to return a new container, which is a modified
copy of the previous container. This applies to operations
like add/remove, but also to things like unmapping memory-mapped
storage, or changing a container's type.
Bitmaps do not support the same copy-on-write semantics,
currently, but "copying" a bitmap and sharing the containers
instead of duplicating them is *much* cheaper than copying
the containers.
Bitmaps do support a .Freeze method, which currently copies
the previous bitmap, making a new one with the same container
pointers, and freezes the individual containers. Use this
if you need a writeable copy of a bitmap -- the resulting
bitmap can safely have its set of containers modified, and
bitmap operators that would want to modify the containers
will use copy-on-write for that.
The primary motivation of this is to reduce the cost of the
row cache used by fragments. As a secondary issue, the row cache
is no longer updated on writes -- that update was actually a
race condition waiting to happen. Rather, writes to a row
invalidate the cache entry for that row. The row cache is
created by creating a new bitmap, and freezing the relevant
containers from the fragment's storage. In the case where
nothing is being written, the row cache grows to contain
bitmaps containing all those containers, but never copies
any containers. If nothing's being read, the row cache is
never created, and the containers are in general not getting
frozen. The only circumstance where copies have to happen is
when things are read (and thus stored in the row cache) and
later modified. In that case, each read freezes objects, and
the first write to a container after it's been frozen will
create a new copy.
We drop the enterprise/b btree implementation, because we
don't really need it anymore -- we now provide that
implementation by default in the open source product anyway.
Along with this, there's a lot of other changes which
improve support for nil containers, as a cheaper representation
for empty containers. Operations which we know will provide
an empty container can always short-circuit and just yield
a nil *Container. Similarly, operations which would provide
a full container can return a single shared full container
object (which is frozen). The higher-level (non type-specific)
container ops are now using that logic to short-circuit
operations for empty and full containers. (For instance,
difference of anything minus an empty container is the
original thing, union of anything and empty is the original
thing, and so on.)
The Containers interface adds "Update" and "UpdateEvery"
methods, based in part on the "Put" interface provided
by the underlying btree implementation; Update performs
a possible update in-place of a container for a given
key, bypassing the need to replicate the search for that
key in the container. UpdateEvery loops through all the
containers.
Containers do not strictly guarantee that they won't
return nil `*Container` objects. However, the container
iterators won't return those -- empty containers aren't
interesting. Some tests are updated to reflect this.
Some of the container internals, like N(), or the isArray()
and related functions, accept nil container pointers. Some,
like Thaw(), do not. For the array(), bitmap(), and runs()
methods, roaringparanoia enables an explicit panic on a nil
container explaining the problem, but the intent is that those
should never be called unless you already know you have the
right kind of container, so by default they don't perform
the extra checks. In most cases, this is already covered
because a nil container is empty, and there's no operation
we can perform that requires us to inspect the contents of
an empty container. This is passing a fair amount of testing,
but the testing may not be comprehensive enough.
The overall impact of this is pretty trivial performance-wise.
In our default roaring/ benchmarks, a few things get a few
percent faster, or slower. The advantage is that, with
read-heavy workloads, the row cache no longer eats up incredible
amounts of memory.
For a smallish test case, pilosa's memory usage (RES in top) after
startup was ~2.5GB. Without this patch, simply reading every
row a few times got memory usage to about 9GB, which seemed
reasonably stable. With this patch, memory usage went to about
3GB. This will be less noticeable in mixed read/write loads,
but it should be consistently significantly lower.
In addition to dropping things from the rowCache on modifications,
we also stopped performing a full count on a modified row when
not using a cache of a kind that would use that count, and don't
repopulate the rowCache regardless. We don't want every write
to imply a corresponding read after it.
There's a lot of room for possible future optimizations in
terms of things like in-place operations, and some of the
row/rowSegment code is a little suspicious to me, but I don't
think it should be *worse* in any cases.
So with the switch to a new linter, we get a lot of new warnings,
and the majority of them are harmless probably, but a few might be
real. Variously just use _ to suppress warnings, or report errors.
There's probably things here that deserve better fixes, but we can
always revisit it.
It used to be useful/desireable to set the other slices to nil when
setting a new slice type, it's no longer useful, take some of those
out.
Also reuse the already-computed run count when converting arrays
and bitmaps to runs.
Noticed in profiling that unmap wasn't being inlined. Also noticed
that every call is on a specific container type, so now they're
specialized and small enough to inline.
This implementation, controlled by the build flag "container24s",
is similar to the single-slice container implementation, but goes
a bit further. First, instead of using a native slice as its internal
storage, it uses pointer/len/cap as distinct values, and only int32
ranges for len and cap. Second, it has a small region of additional
storage which it uses as a backing store by default for arrays or
runs. The idea is that, if you request a new empty array container,
you get one with a pre-allocated virtual slice big enough for five
values, actually stored in the Container. This is useful because
Go's allocator has size classes for 16 and 32 bytes, and the
Container comes in at 24 bytes worth of storage -- meaning that if
you allocate a container, you're allocating 32 bytes anyway, so we
might as well use that space to avoid extra allocations.
This includes some test fixups because DeepEqual was testing
too much equality in some tests.
Also, we simplify unionArrayArray to postpone creating a Container
until we're ready.
On a 64-bit machine, the slices in a Container consume 72
bytes, and the Container itself is 80. But we only use one
slice at a time! This patch shifts us to keeping a single
slice in the Container, and converting provided slices to
and from that type when we want to update it. (It is not
safe to access the slice through the wrong type.)
We also add some new tests, conditional on a build tag
called `roaringparanoia`. These tests will be optimized
away entirely by the compiler when the tag isn't
present, because the conditionals use a const. These catch
possible errors like trying to access the bitmap slice
of a non-bitmap container.
We also eliminate all direct creation of Container literals,
so we can mess with the internals more. (On reflection
and study, we decided not to go to the fancier design where
references to .n and .typ were also converted to function
calls, which would have allowed packing those attributes
more tightly, because it was a lot more overhead and a lot
of work to keep track of.)
There's some circumstances where we appear to have been
relying on incorrect guesses about the nature of containers.
For instance, in xorBitmapRun, there's logic that makes sense
only if the output's a run container, but it's not, it's a
bitmap container. This creates strange behavior sometimes,
though. Several of these are corrected now.
Add a benchmark to test a specific case where UnionInPlace is
underperforming the naive union operation badly.
Also, the UnionBulk test was reusing a bitmap, meaning that it ended
up doing a lot of unions into a bitmap that already had all the
bits it was supposed to have. This broke a couple of other tests
in unexpected ways.
We also now use UnionInPlace in importRoaring, and test it
in the container combinations tests via a wrapper.
confirm that the number of runs comes out as expected,
and add a couple of numbers out of order to verify that
the 17-18-19 set gets coalesced into one run even
if we add 17 and 19 before 18.
bitmapEquals isn't currently being called ever, but it has
an arcane edge-case bug, so I've made the test case for it
and commented it out for future reference.
