Optimize fetching individual records on each cache type #622
Conversation
Th3-M4jor
force-pushed
the
optimize-basic-get-by-id-all-caches
branch
from
db8474f
to
cf4adc4
Compare
There was a problem hiding this comment.
Thanks for the PR, now we've gone full circle!
It's a bit sad, but inspecting the QLC output I see where it's coming from, whilst QLC provides lookup functions for primary keys it seems the optimizer doesn't work for us :-(
Can you remove the no longer called functions from the *_qlc.erl modules as well?
Co-authored-by: jchristgit <[email protected]>
|
Hello, I'm curious: couldn't you give QLC a more efficient match spec via the the |
|
Well, what confuses me about this is that I checked the queries in question @Th3-M4jor do you maybe have the |
|
I had the user execute handle = :nostrum_guild_cache_qlc.get(<put a real guild id here>, Nostrum.Cache.GuildCache.Mnesia)
:io.format(~c"~s~n", [:qlc.info(handle)])And this was the result: qlc:q([
Guild ||
{GuildId, Guild} <-
mnesia:table(nostrum_guilds,
[{n_objects, 100},
{lock, read} |
{traverse,
{select,
[{{'_', '$1', '$2'},
[],
[{{'$1', '$2'}}]}]}}]),
GuildId =:= RequestedGuildId
]) |
|
Okay this is even more confusing. I checked against the ETS guild cache implementation and got this: ets:match_spec_run(ets:lookup(nostrum_guilds, 377642134112567296),
ets:match_spec_compile([{{'$1', '$2'}, [], ['$2']}]))That's entirely as expected, plenty fast. But when using Mnesia it does do qlc:q([
Guild ||
{GuildId, Guild} <-
mnesia:table(nostrum_guilds,
[{n_objects, 100},
{lock, read} |
{traverse,
{select,
[{{'_', '$1', '$2'},
[],
[{{'$1', '$2'}}]}]}}]),
GuildId =:= RequestedGuildId
]) |
|
Hm, yeah, I saw this locally as well but I didn't yet gather that the
mnesia caching backend doesn't just "traverse" in a smart way, but it
forces a full list comprehension evaluation, as apparent in your
example.
The problem seems to be that we pull out the guild ID and guild from the
table (see the QLC file) and by calling `mnesia:table` like that, we
effectively disable efficient key lookups:
https://github.com/erlang/otp/blob/master/lib/mnesia/src/mnesia.erl#L5091-L5103
The root of the problem is the `query_handle` function in
`Nostrum.Cache.GuildCache.Mnesia`, which is effectively there to remove
the tuple tag from the first table element. Proposed solutions:
- Stop mnesia from requiring the first element to be an atom and get rid
of it (hard?)
- Teach mnesia / QLC to recognize table traverse functions and still
utilize lookup functions if they are used. I think for simple cases,
like ours, this should be doable, line 5093 of mnesia.erl could
be adjusted to recognize "column shifts" like ours and still optimize
it for a lookup function. (doable depending on data structure in `Trav`)
- Figure out another way to write the QLC query that allows it to be
optimized without breaking the expected user (hard?)
In either case though I don't want disgruntled user, and if we end up
contributing something to OTP then it's likely a while before we can use
it. So the question is, how badly is the user impacted by it? Is it an
inconvenience or a larger bug? I would be happy to merge this and make a
patch release.
|
|
@jchristgit Hi! User in question here. The bug is relatively simple to work around (just use Mnesia directly instead of the |
QLC queries via mnesia-based caches that would use the `{traverse,
{select, MatchSpec}}` in any shape or form would cause the QLC query to
be executed in two parts, the `mnesia:table` call running the entire
table over the selected match specification, and then another Erlang
list comprehension that would filter the results of that across a list
comprehension in plain Erlang. Per the discussion in #622, this is how
such a query would look like under `qlc:info/1`:
qlc:q([
Guild ||
{GuildId, Guild} <-
mnesia:table(nostrum_guilds,
[{n_objects, 100},
{lock, read} |
{traverse,
{select,
[{{'_', '$1', '$2'},
[],
[{{'$1', '$2'}}]}]}}]),
GuildId =:= RequestedGuildId
])
The issue is that neither QLC nor mnesia can cleanly optimize it here:
Mnesia does not know about the condition specified in QLC, and QLC's
optimization to use a `lookup_fun` is knocked out by the fact that it
can't reach into the traverse call to detect the reordered columns. It
might be possible to implement this in QLC itself, given it is smart
enough to figure out when to use the lookup function based on the
indices, together with some cooperation from Mnesia itself. Obviously,
this behaviour would lead to unacceptable performance.
This commit introduces an optimization that allows guild, member and
presence cache implementations to export a `query_handle/1` function
that accepts a match specification guard, that is, the "middle part" of
a match specification. The guard determines which rows shall be
filtered. Do note, however, that this is still unable to perform a
complete optimization of lookups of single records - it will still
traverse the table, but in the native ETS code.
|
@atlas-oc thanks for chiming in. Yup, I agree, this is absolutely not intended behaviour. I've digged pretty deep into the issue and opened #625 which hits an optimization point somewhere inbetween "loads the entire list into memory" and "O(1) query". More specifically, it runs the match specification in ETS directly. More information in that PR. That being said, while the PR would help with this case, for single-instance lookups it is still slow. So all-in-all I believe that we should do both: I will review this PR tomorrow, merge it, then update the other PR and merge it and then on the weekend I can hopefully come around to documenting it & cutting a release with both. |
|
I admittedly have no familiarity with QLC, but is there any particular reason why we're using it as an abstraction here instead of expanding the "base" Cache modules to delegate more functionality to each implementation, such as |
|
Thanks! |
|
The intention is that you define that function once and
then you don't really need to care about how nostrum queries the cache
or will query the cache in the future. Any future amendments to the
query cache can just utilize the QLC handle, and QLC can optimize
lookups on fields. With mnesia for instance, we can optimize lookups in
multiple positions if indices exist, because mnesia informs QLC about
them.
The issue is that with the built-in handles specifically it's a bit
tough to work with these optimization troubles. Mnesia in particular
always has an atom in the first position (cause it expects to work with
records in its table), which we need to filter out to match the expected
query handle format. Exactly that is unfortunately why the optimization
fails.
I've been working on investigating the performance regression upstream,
because I still think that it should be possible to optimize it to use
the lookup function for simple cases like this. Unfortunately the QLC
source code is something built by higher powers that I haven't managed
to speak to yet...
|
It was reported that for bots in a large number of servers it takes sometimes half a second to fetch a user from the cache under the current
:qlcbased method as it will do a full table scan.As a workaround, functions which only return a single record by its primary key will be optimized to avoid
:qlc