fix: make compression_level reachable - #50
Merged
MikaAK merged 1 commit intoAug 16, 2026
Conversation
The option was documented and dead. No adapter declares it, so passing it in `opts:` made `NimbleOptions.validate!/2` reject the whole cache at compile time; and on the paths where opts resolve at runtime the macro's `@compression_level` was only read from a compile-time list, so it stayed nil and never reached the encoder. There was no configuration that produced a compressed value. It is an encoder concern rather than a store concern, so it now lives on the `use Cache` line and is taken off the adapter opts before they are validated — an adapter that forwards its opts to the store would choke on an option that was never meant for it. The `opts: [compression_level: 6]` spelling keeps working. Setting it also forces encoding on the adapters that hold terms natively. Asking for compression is asking for bytes, and silently handing back an uncompressed term is how this option got lost in the first place. Strategy adapters encode inside the strategy module, through what they wrap, so the option cannot reach an encoder from there. They raise at compile time now instead of accepting it and doing nothing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## perf/adapter-aware-term-encoding #50 +/- ##
===================================================================
Coverage ? 83.40%
===================================================================
Files ? 23
Lines ? 663
Branches ? 0
===================================================================
Hits ? 553
Misses ? 110
Partials ? 0 ☔ View full report in Codecov by Harness. |
MikaAK
added a commit
that referenced
this pull request
Aug 16, 2026
The option was documented and dead. No adapter declares it, so passing it in `opts:` made `NimbleOptions.validate!/2` reject the whole cache at compile time; and on the paths where opts resolve at runtime the macro's `@compression_level` was only read from a compile-time list, so it stayed nil and never reached the encoder. There was no configuration that produced a compressed value. It is an encoder concern rather than a store concern, so it now lives on the `use Cache` line and is taken off the adapter opts before they are validated — an adapter that forwards its opts to the store would choke on an option that was never meant for it. The `opts: [compression_level: 6]` spelling keeps working. Setting it also forces encoding on the adapters that hold terms natively. Asking for compression is asking for bytes, and silently handing back an uncompressed term is how this option got lost in the first place. Strategy adapters encode inside the strategy module, through what they wrap, so the option cannot reach an encoder from there. They raise at compile time now instead of accepting it and doing nothing.
MikaAK
added a commit
that referenced
this pull request
Aug 16, 2026
* perf: stop encoding terms for adapters that store them natively
Cache.TermEncoder.encode/2 ran on every put and decode/1 on every get, in
the generic Cache macro, for every adapter. Most adapters store Erlang
terms natively, so the round trip was pure overhead — and the decode
dominated the lookup it was attached to. On a 500k-entry ETS table, a bare
:ets.lookup of a ~10KB body costs 63 ns while binary_to_term of the same
body costs ~29,000 ns.
Adapters now declare how they store values via an optional
native_term_storage?/1 callback. Cache resolves it at compile time from the
adapter module attribute, so there is no runtime branch on the hot path.
ETS, Agent, PersistentTerm, ConCache and Counter store terms; Redis stores
bytes and DETS owns a durable on-disk format, so both keep encoding. The
callback is optional and defaults to encoding, so third-party adapters are
unaffected.
ETS with :rehydration_path also keeps encoding — it dumps the table with
:ets.tab2file/2 and reads it back on the next boot, so it is a durable
format with the same backward-compatibility constraint as DETS.
HashRing and RefreshAhead call the encoder directly, bypassing the macro, so
they resolve the capability against the adapter they wrap. MultiLayer needed
no change: each layer is a full Cache module that owns its own encoding, so a
[ETS, Redis] stack already stores a term in ETS and bytes in Redis.
Benchmarks (500k entries, ~10KB body, 20k iterations):
get/1 29,981 ns -> 6,211 ns (4.8x)
put/2 23,542 ns -> 10,804 ns (2.2x)
The win holds across payload sizes — a small map goes from 2,105 ns to 302 ns.
This also fixes three bugs that were consequences of encoding unconditionally:
* ConCache.get_or_store/3 wrote through ConCache directly, bypassing the
encode in put/3, so a later get/1 raised trying to binary_to_term/1 a
raw term.
* decode/1 used Jason.decode!/1 on any brace-wrapped binary, so a value
like "{oops}" raised Jason.DecodeError on read. It now falls back to
returning the binary unchanged.
* The raw ETS API this adapter exposes (match_object/1, select/1,
tab2list/0, foldl/2) saw opaque encoded binaries instead of the terms
that were put.
PersistentTerm also regains the zero-copy read it exists for.
* fix(test): drain the in-flight refresh task before releasing the global lock
The refresh-ahead lock test released the :global lock immediately after a get
that had just spawned a refresh task. That task was still racing toward
:global.set_lock, so it could reach it after the del_lock, take the freed
lock, and refresh the value the very next assertion expects to be untouched —
failing with "locked:locked_key" where "original" was expected.
The race was always there; making get/1 faster widened the window by getting
the test process to del_lock sooner, and it surfaced on a loaded CI runner.
Wait for the spawned task to lose the race and clean itself up while the lock
is still held, so nothing is in flight when the lock is released.
* fix: keep encoding for strategies that hand values to another node
`Cache.HashRing` rpcs the stored value to the node that owns the key, and
`Cache.MultiLayer` under `broadcast_mode: :replicate` pushes it to the other
nodes' layers. Both were resolving `native_term_storage?` against the adapter
they wrap, so a `Cache.ETS` ring stopped encoding — which changes the wire
format between nodes.
That is only safe if every node upgrades at once. During a rolling deploy the
two versions read each other's writes for the same key: a 0.4.x writer leaves a
`term_to_binary/1` blob that this version hands back to the caller unchanged,
and a value written by this version blows up in the old `binary_to_term/1` when
it happens to be a binary.
So both strategies keep encoding, and `Cache.HashRing` is reverted to what it
does on main. Their format is unchanged from 0.4.x and a mixed-version cluster
stays safe. Node-local adapters are untouched and keep the win.
* test: pin the invariant that makes an upgrade safe
Skipping the encode is only safe where the previous version's bytes cannot
survive to be read by this one — which means the value has to die with the VM
that wrote it. Everything that reaches a disk, a Redis server or another node
keeps the 0.4.x representation.
That reasoning was spread across per-adapter docs, so a new adapter or a new
persistence option could quietly break it and nothing would fail. These two
tests state it directly: durable stores encode, volatile stores do not.
Also covers a Redis key written by an older version, which had no test — the
DETS equivalent did.
* fix: cache a JSON string and get a string back (#51)
`encode/2` checked binaries against `~r/^{.*}$/` and stored the brace-wrapped ones
unencoded, which left `decode/1` guessing what it was looking at. Caching a JSON
string returned a decoded map — a String in, a Map out, silently:
put(:k, ~s({"a": 1})) |> get(:k) #=> {:ok, %{"a" => 1}}
The guess had no way to be right. A binary that happens to look like JSON is
indistinguishable from a serialised value once the shape of the payload is the
only evidence available.
So there is no guess now. Binaries are encoded like every other term, and
`decode/1` keys off the external term format version byte, which every value
this library encodes carries and no raw payload does.
Bytes that are not in that format were written by an earlier version or by
something other than this library, so they keep their previous reading: a raw
JSON string still decodes to a map, a raw digit string to an integer. Existing
Redis and DETS data is unaffected — only values written from here on are
type-stable.
Integers still go to the store as integers, so a store that understands numbers
still sees one, and `Cache.Redis.Hash` field names are untouched.
`decode/1` also stops raising on a binary that is not an encoded term. It used
to reach `binary_to_term/1` for anything that was not digits or brace-wrapped,
which is an ArgumentError on any value another tool wrote into the store.
* fix: make compression_level reachable (#50)
The option was documented and dead. No adapter declares it, so passing it in
`opts:` made `NimbleOptions.validate!/2` reject the whole cache at compile time;
and on the paths where opts resolve at runtime the macro's `@compression_level`
was only read from a compile-time list, so it stayed nil and never reached the
encoder. There was no configuration that produced a compressed value.
It is an encoder concern rather than a store concern, so it now lives on the
`use Cache` line and is taken off the adapter opts before they are validated —
an adapter that forwards its opts to the store would choke on an option that was
never meant for it. The `opts: [compression_level: 6]` spelling keeps working.
Setting it also forces encoding on the adapters that hold terms natively. Asking
for compression is asking for bytes, and silently handing back an uncompressed
term is how this option got lost in the first place.
Strategy adapters encode inside the strategy module, through what they wrap, so
the option cannot reach an encoder from there. They raise at compile time now
instead of accepting it and doing nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #48 — base it back to
mainonce that merges.The problem
compression_levelis documented (README,Cache.TermEncoder.encode/2takes it) and dead. There is no configuration that produces a compressed value:opts_definition/0, soNimbleOptions.validate!/2rejects the whole cache module at compile time:{app, key}, MFA, zero-arity fun) — the macro only reads@compression_levelwhen the opts are a compile-time list, so it resolves toniland never reaches the encoder.Every path is closed. That is also why the first spelling above is a compile error rather than a silent no-op: nobody can currently be relying on it.
The change
The level belongs to the encoder, not to the store, so it moves to the
use Cacheline:opts: [compression_level: 6]works too. This matters beyond validation:Cache.ETSforwards leftover opts to:ets.new/2, which would fail on an option that was never meant for it.Tests
test/cache/compression_level_test.exs— 5 tests: compression through theuseline, through the legacyopts:spelling, on a byte-storing adapter (Redis), the no-compression control, and the strategy raise. Each asserts the stored bytes are smaller than a plainterm_to_binary/1of the same value and that it still round-trips.Not vacuous — on
mainthe test file does not compile, with exactly theunknown options [:compression_level]error above.Not in scope
Cache.Redis.Hashfield encoding andCache.Redis.Setmembers readopts[:compression_level]from adapter opts that reach them through the adapter, not through this path. They are unchanged and stay uncompressed, same as today.