A few considerations in case you need to work on performance even further (you may not even need them, but it's worth knowing).
- Using the
GenServer
flow to read data may become a bottleneck (as it happens inside theGenServer
process) - If you don't care about duplicates, you may set the table as
duplicate_bag
. This speeds up writes, as it doesn't have to check for dupes.
It may also be interesting to read directly from the table instead of going through the GenServer
.
That's possible by creating the table as public
and named_table
, giving it __MODULE__
as name.
In addition, as the table would be mostly reads, it can be marked as read_concurrency
(which optimizes reads over writes).
ets.new(__MODULE__, [:named_table, :duplicate_bag, :public, :read_concurrency])
This setup allows reading directly from it in fetch/2
by referencing the table by name without needing call/2
. The read therefore happens in the calling process.
A side effect of this is that it also reduces the possibility of a table crash (the owner is still the GenServer
).
defp get(slug) do
case :ets.lookup(__MODULE__, slug) do
[] -> {:not_found}
[{_slug, result}] -> {:found, result}
end
end
Note that the table is now public, so any process can read it (so no private data).
Great results!
I've looked at the linked commit and here are my thoughts:
{:ok, self()}
onstart_link/1
means that the controller process (in our case the Supervisor) owns the table. Not only that, it ends up supervising itself. I'm not sure how that works, I should check it but I don't think that a process can trap its own exit. In other words, theLinkCache.Supervisor
tree doesn't give you much.RedirectController
, so any failure would cause the request to fail. So if you fail to write to the cache, your request fails.Considering the above, I would test an approach where:
LinkCache.Supervisor
.LinkCache
comes back to be aGenServer
.set/2
invokes a cast that writes to the table asynchronously. This is because in the current implementation there's no check at all on the success of the insertion, so there's no downside to make it asynchronous. It's a cache after all, so worst case scenario we lose that value (and logs would catch that failure).Hope it's clear!
Keep up the good work :)