* refactor: avoid `tokio::spawn` for cache requests
During the happy-path, we don't need any tokio task to drive a cache
loader requests because the future issuing the request just act as a
driver. If that future is cancelled, we place the cache request in an
extra task. This avoid latencies due to task overhead and (task) context
switches for most requests. This may remove a millisecond or two from
latency but also makes the whole thing easier to analyze/profile because
we don't spawn a truckload of tasks.
This trick was borrowed from rskafka.
* refactor: split up code
* feat: rework cache refresh logic
Instead of issuing a single refresh when a GET request for a cached key
comes in, start a background job (using some efficient logic to not
overload tokio) per key that refreshes the key using some exponential
backoff. The timer is reset a new GET request comes in. This has the
following advantages:
- our backoff logic decorrelates the requests
- the longer a key was not used, the less often it will be updated
All test (esp. integration tests) as adjusted accordingly, mostly to
account for the fact that no extra GET is required to start the refresh
timer.
Closes#5720.
* docs: improve
Co-authored-by: Andrew Lamb <alamb@influxdata.com>
* refactor: simplify rng overwrite
Co-authored-by: Andrew Lamb <alamb@influxdata.com>
Policy subscribers basically never store `V` (the cached value), so we
should not clone that one unconditionally. But even `K` (the cache key)
is not stored in all cases. So let's pass a reference for both and let
the policies decide if they wanna clone the data or not.
* test: improve test assertion
* refactor: LRU SETs should NOT be treated as "used"
Improves interaction of refresh and LRU.
* docs: improve
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor: improve consistent access under "remove if"
With all the concurrency introduced in #5668, we should be a bit more
careful with our "remove if" handling, esp. if a removal is triggered
while a load is running concurrently. This change introduces as
`remove_if_and_get` helper that ensures this and the querier over to use
it. The parquet file and tombstone caches required a bit of a larger
change because there the invalidation and the actual GET were kinda
separate. We had this separation for the other caches as well at some
point and decided that this easily leads to API misuse, so I took this
opportunity to "fix" the parquet file and tombstone cache as well.
* docs: improve
* refactor: use concrete backend type in `ChangeRequest`
* refactor: "remove if"-checks shall NOT count as "used"
* test: improve docs
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Criterion comes with some extra cargo tooling called `cargo criterion`
which can be used instead of `cargo bench`. The advantage is that we
don't need to compile the entire reporting infrastructure into our
benchmarks. So let's embrace this separation of concerns.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* feat: naive `AddresableHeap::update_order`
* refactor: use `update_order` within LRU policy
* test: add benchmark for `AddressableHeap::update_order`
* refactor: avoid double-hash when updating addressable heap orders
```text
update_order_existing_after_n_elements/1
time: [25.483 ns 25.513 ns 25.547 ns]
change: [-42.490% -42.365% -42.247%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
update_order_existing_after_n_elements/10
time: [68.158 ns 68.211 ns 68.266 ns]
change: [-19.391% -19.131% -18.952%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
update_order_existing_after_n_elements/100
time: [128.10 ns 128.43 ns 128.83 ns]
change: [-17.732% -17.531% -17.255%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe
update_order_existing_after_n_elements/1000
time: [223.08 ns 224.06 ns 225.30 ns]
change: [-9.0635% -8.5828% -7.9794%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
2 (2.00%) high mild
5 (5.00%) high severe
update_order_existing_after_n_elements/10000
time: [1.0032 µs 1.0216 µs 1.0402 µs]
change: [-6.0920% -3.7038% -1.0826%] (p = 0.01 < 0.05)
Performance has improved.
update_order_new_after_n_elements/0
time: [35.898 ns 35.919 ns 35.943 ns]
change: [+183.39% +183.77% +184.12%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
update_order_new_after_n_elements/1
time: [13.273 ns 13.299 ns 13.344 ns]
change: [-6.6980% -5.9798% -5.2633%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe
update_order_new_after_n_elements/10
time: [14.010 ns 14.084 ns 14.183 ns]
change: [-13.579% -13.117% -12.553%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) high mild
9 (9.00%) high severe
update_order_new_after_n_elements/100
time: [23.846 ns 23.883 ns 23.921 ns]
change: [-4.7412% -4.3738% -4.0715%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild
update_order_new_after_n_elements/1000
time: [28.590 ns 28.646 ns 28.705 ns]
change: [-4.1597% -3.6132% -3.0701%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high mild
update_order_new_after_n_elements/10000
time: [31.459 ns 31.975 ns 32.601 ns]
change: [-32.153% -20.689% -11.961%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
5 (5.00%) high mild
2 (2.00%) high severe
````
Improvements might be even bigger for more expensive hash functions
(e.g. for `K = Arc<str>`).
Note that there is one outlier: `update_order_new_after_n_elements/0`. I
suspect this is due to slightly different compiler decisions (there is
no technical difference for "update a key of an empty heap"). Since this
case is also pretty uncommon in practice (only ~once when the process
boots up), I deem this acceptable.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Instead of a naive "remove + insert", use a proper insertion routine
that touches the hash map only once.
Note that in case of an override (i.e. the entry with this key already
existed) we need to touch the heap twice, because the sort order likely
changed (we don't optimize the "identical order" case here because it is
pretty unlikely that this will happen in practice).
**Perf results:**
```text
insert_n_elements/0 time: [16.489 ns 16.497 ns 16.506 ns]
change: [-8.1154% -7.9967% -7.8990%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) low mild
1 (1.00%) high mild
2 (2.00%) high severe
insert_n_elements/1 time: [59.806 ns 59.839 ns 59.875 ns]
change: [-14.241% -14.160% -14.086%] (p = 0.00 < 0.05)
Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
insert_n_elements/10 time: [601.58 ns 602.26 ns 603.09 ns]
change: [-20.870% -20.714% -20.565%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
4 (4.00%) high mild
5 (5.00%) high severe
insert_n_elements/100 time: [6.9096 µs 6.9161 µs 6.9246 µs]
change: [-18.759% -18.667% -18.553%] (p = 0.00 < 0.05)
Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe
insert_n_elements/1000 time: [107.71 µs 107.76 µs 107.82 µs]
change: [-14.564% -14.427% -14.295%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
insert_n_elements/10000 time: [2.8642 ms 2.8700 ms 2.8765 ms]
change: [-11.079% -10.860% -10.605%] (p = 0.00 < 0.05)
Performance has improved.
Found 15 outliers among 100 measurements (15.00%)
15 (15.00%) high severe
````
Note that the results are even better for keys with more expansive hash
functions (we have few in the querier).
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* ci: use same feature set in `build_dev` and `build_release`
* ci: also enable unstable tokio for `build_dev`
* chore: update tokio to 1.21 (to fix console-subscriber 0.1.8
* fix: "must use"
This is similar to #5530. Do not box the function because the user will
likely use it as `Box<dyn ResourceEstimator<...>>` or
`Arc<dyn ResourceEstimator<...>>`.
The slightly larger code change is because the clean-up around `TestSize`
which helps with the doctests (and also we had two copies of that type).
There's no technical reason to dyn-dispatch the loader. The user may
free to do so however.
Note that there's no change within `querier` because the type
inference will now automatically treat the passed `Arc` as `Arc<L>`
insteado of `Arc<dyn Loader<...>>`.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor: do not always box `FunctionLoader`
While constructing our caches, we rarely spell out types. Instead we
just "box" the end result (`Box<dyn Cache<...>>`) before storing it
within the struct (e.g. `NamespaceCache`).
This means that `FunctionLoader` does NOT need to erase the type of the
function it contains. This saves us a `Box` which means less pointer
chasing and more room for the compiler to optimize.
* docs: typos
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
The API user may still use a `Box<dyn ...>` if they want, but they
technically don't have to.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
The API user still CAN use dynamic dispatch but doesn't have to. This
also simplifies the generics a bit.
This is similar to #5520.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
This removes some `Box<dyn ...>` indirection when the user doesn't want
it (you still can, but don't have to) and makes the whole type handling
easier to understand.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor: use a single timestamp in policy backend
Prior to this PR we had at least 1 `TimeProvider::now` calls per GET
request (for caches that only used LRU) and up to 3 calls (caches with
LRU + refresh + TTL). Let's instead use a single timestamp that is
created by the policy backend itself (instead of the policies). This has
the following consequences:
- **efficiency:** `SystemProvider::now` is not free, even though under Linux
this doesn't result in a syscall, it uses the stdlib time system which
also checks for monotonicity
- **consistency:** All changes for a single trigger (e.g. a
GET cache call) now use a single timestamp instead of slightly
increasing ones. I argue this is the better semantic, simpler to
understand and better to debug.
For some (slightly artificial) local performance experiment, this shaves
off around 2ms per single-table SQL query. However I expect that there might
be more degenerated cases (e.g. multi-table SQL queries or some
InfluxRPC requests that hit multiple tables).
The majority of this patch is moving the `TimeProvider` from the
policies into the policy backend.
* docs: explain `now` parameter
* feat: refresh policy for caches
For #5318 we want to have a policy that refreshes keys before they are
too old. I initially tried to fold both TTL and the refresh system into
a single policy but than decided that they will basically be two
policies in one with a harder-to-test interface. Semantically TTL and
refresh are also a bit different (but will usually be used together):
- **TTL:** Prevents that a users gets data that is too old. It is some kind
of "soft correctness". In some sense this is related to the "remove
if" policy where some part of the system knows for sure (or with
reasonable likelyhood) that a cache entry is outdated. Note that TTL's
primary job is NOT to clean up memory from old keys (even though it
indirectly does that). There is no reason cached entries should be
removed except for correctness (TTL and remove-if) or resource
pressure -- and the latter is handled by the LRU policy.
- **Refresh:** While TTL is some kind of deadline, we often have good
reason to refresh the key before we pull the plug, namely when an
entry is used and a bit old (but not too old). The concrete mechanism
to archive this is flexible. At the moment the policy is rather
simple -- just start a refresh task if a key is old and we receive a
GET request -- but can be extended in the future.
This also adds some integration tests for TTL+refresh. There will be
follow-up changes to test the interaction with LRU as well, althouh I am
pretty certain that there won't be any surprises due to the excessive
testing we have in place for the policy backend itself as well as all
the policies.
This change also does NOT integrate the refresh with the querier for the
sake of keeping the changeset "small" (i.e. it is already rather large).
* docs: improve
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* refactor: allow `ChangeRequest` to carry a lifetime
Let's not restrict our change functions to `'static` because this would
require us to clone loads of data to achieve predicate-based
`remove_if`.
* refactor: convert `remove_if` feature to policy framework
Decided to drop the "shared" functionality. We only use the small
`remove_if` bit which is way easier to reason about.
For #5320.
* refactor: address review comments
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor: port TTL backend to policy framework
Note that this is "just" a port, it does NOT change how TTL works. This
will be done in #5318.
Helps with #5320.
* fix: ensure inner backend is empty
* test: add some smoke test
* refactor: abstract change requests for cache policies
Tl;Dr; Lets replace the hard-coded enum of change requests with a more
flexible and easier-to-understand function-based approach. It also
unifies the interface for "initial requests" and "reactions".
Story time:
I just wanted to port over the "shared" backend with its "remove if"
functionality to the policy framework. Turns out that this backend does
not just use simple atomic operations but is basically a
"compare&exchange"-like operation (technically: GET+REMOVE) that relies
on a single mutable access lifetime to make sure that this behavior is
sane and nobody messes with the value between the GET and REMOVE. I call
this a "compound request".
The new policy framework did not support this kind of requests so I
thought how I could shoehorn it in. Basically I wanted to extend
`ChangeRequest` to support these kind of operations. But listing
combinations explicitly kinda seemed like some hack. So my brain slowly
came up with an abstract approach how to encode these operations and
after a while I was close to design some kind of mini-VM. I also
considered some ugly workarounds or to not express the "remove if"
functionality" as a policy. My nice vision of a policy framework started
to crack...
This is when sanity kicked in and I realized: every change request is
technically just a function on a (virtual) cache backend. Sure,
all-powerful Rust functions allow you to do ugly stuff that will result
in deadlocks, but with good docs and helpers for the common operations
the risk is very low. Furthermore this is already close to the system I
had for the "initial requests" and which I called a "callback backend".
So I unified both systems and made the change request abstract and all
tests pass and I think it is the better design.
Helps with #5320.
* docs: typos
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
1. add GET support for the subscribers (this is needed so that
TTL/refreshers and LRU systems "know" when a key was used)
2. improve multi-threading test to not rely on a wait loop but use a 2nd
barrier instead
3. ensure that panics in the testing background thread are propagated
and make the test fail
4. implement defaults for `Subscriber` methods to reduce boilerplate for
implementators
Helps with #5320.
* feat: new policy system for caches
This is the framework part for the policy system outlined in #5320. It
does NOT port any existing policies (likely TTL, LRU and shared) over to
the new system. This will be a follow-up.
* docs: improve example
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* feat: cache tracing
Add tracing to the metrics cache wrapper. The extra arguments for GET
and PEEK make this quite simple, because the wrapper can just extend the
inner args with the trace information.
We currently terminate the span in `querier::cache` (i.e. only pass in
`None`, so no tracing will occur) to keep this PR rather small. This
will be changed in subsequent PRs.
For #5129.
* fix: typo
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
This prepares the test system for the usage of extra args for GET and
PEEK by pulling all generics and the whole interface into a single
trait. This is similar to how the write buffer tests work.
This is needed to introduce extra args into the metrics wrapper (i.e. to
pass down spans) while still being able to use the generic tests in a
follow-up PR. Overall this change is required for #5129.
The tests itself are unchanged.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
This will be used to pass spans down to `CacheWithMetrics` (or a new
wrapper specific to tracing) and will help with #5129.
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Removes an erroneous line and reworks one sentence for clarity.
Co-authored-by: pierwill <pierwill@users.noreply.github.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* chore(arrow_util): readability improvements
Signed-off-by: Ryan Russell <git@ryanrussell.org>
* chore(tracker): readability improvements
Signed-off-by: Ryan Russell <git@ryanrussell.org>
* chore(cache_system): improve readability
Signed-off-by: Ryan Russell <git@ryanrussell.org>
* refactor(lru test): rename `test_panic_id_collision`
Signed-off-by: Ryan Russell <git@ryanrussell.org>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
To roughly gauge how much data we re-load into cached (i.e. data that
was already loaded but was later evicted due to LRU pressure or TTL
eviction) this change introduces a new metric that estimates if a cache
entry that is requested from the loader was already seen before (using a
probabilistic filter).