Improve caching #113

masklinn · 2022-05-01T17:33:46Z

Adds caching to all Parse* functions, using a unified cache (so the
functions can be intermixed, and the cache size applies to everything).
Increase default size of cache, 20 seems very small, 200 doesn't
seem too big (though it's completely arbitrary). Tried to play
around with random samplings (using non-linear distributions) and
it didn't exactly change anything but...
Updated the cache replacement policy to use a FIFO-ish policy.

Unified Cache

Split the parsers between a caching frontend and a "raw" backend (the
old behaviour) to avoid Parse having to pay for multiple cache lookups
in order to fill entries.

Also decided to have the entries be handed out initialized, so
_lookup always returns an entry, which can be partial. The caller
is to check whether the key they handle (for specialised parsers) or
all the sub-keys are filled, and fill them if necessary. This makes
for relatively straightforward code even if it bounces around a
bit. The unified cache makes it so the functions are intermixed and
benefit from one another's activity.

Also added a **jsParseBits parameter to ParseDevice: I guess that's
basically never used, but Parse forwarded its own jsParseBits,
which would have led to a TypeError.

Cache Policy

The cache now uses a FIFO policy (similar to recent updates to the
stdlib's re module) thanks to dict being ordered since 3.6. In my
limited bench not showing much (possibly because the workload was so
artificial) LRU didn't stat much better than FIFO (based on hit/miss
ratios) and FIFO is simpler so for now, FIFO it is. That's easy to
change anyway.

Anyway the point was mostly that any cache which doesn't blow the
entire thing when full is almost certain to be an improvement.

A related change is that the cache used to be blown after it had
MAX_CACHE_SIZE+1 entries, as it was cleared

on a cache miss
if the cache size was strictly larger than MAX_CACHE_SIZE.

Meaning the effective size of the cache was 21 (which is a pretty
large increment given the small cache size).

This has been changed to top out at MAX_CACHE_SIZE.

- Adds caching to all Parse* functions, using a unified cache (so the functions can be intermixed, and the cache size applies to everything). - Increase default size of cache, 20 seems very small, 200 doesn't seem too big (though it's completely arbitrary). Tried to play around with random samplings (using non-linear distributions) and it didn't exactly change anything but... - Updated the cache replacement policy to use a FIFO-ish policy. Unified Cache ============= Split the parsers between a caching frontend and a "raw" backend (the old behaviour) to avoid Parse having to pay for multiple cache lookups in order to fill entries. Also decided to have the entries be handed out initialized, so `_lookup` *always* returns an entry, which can be partial. The caller is to check whether the key they handle (for specialised parsers) or all the sub-keys are filled, and fill them if necessary. This makes for relatively straightforward code even if it bounces around a bit. The unified cache makes it so the functions are intermixed and benefit from one another's activity. Also added a `**jsParseBits` parameter to `ParseDevice`: I guess that's basically never used, but `Parse` forwarded its own `jsParseBits`, which would have led to a `TypeError`. Cache Policy ============ The cache now uses a FIFO policy (similar to recent updates to the stdlib's re module) thanks to dict being ordered since 3.6. In my limited bench not showing much (possibly because the workload was so artificial) LRU didn't stat much better than FIFO (based on hit/miss ratios) and FIFO is simpler so for now, FIFO it is. That's easy to change anyway. Anyway the point was mostly that any cache which doesn't blow the entire thing when full is almost certain to be an improvement. A related change is that the cache used to be blown after it had MAX_CACHE_SIZE+1 entries, as it was cleared - on a cache miss - if the cache size was strictly larger than `MAX_CACHE_SIZE`. Meaning the effective size of the cache was 21 (which is a pretty large increment given the small cache size). This has been changed to top out at `MAX_CACHE_SIZE`. Fixes ua-parser#97

Amongst other changes, ua-parser#113 switched the cache to a FIFO inspired by the standard library's re module, however it didn't really take concurrency in account, so didn't really consider: that double-pops are possible (probably why the stdlib ignores a bunch of errors), which can cause KeyError during lookup (as two workers try to clear the first key, one succeeds, and the other doesn't find the key and fails). It also has a few other less major issues: - double-inserts are possible, which can cause the cache to exceed set capacity permanently by the number of concurrent workers - the stdlib's method only works properly with Python 3.6's naturally ordered `dict`, but I'd rather not drop 2.7 compatibility from 0.x unless there are very good causes to as, despite 2.7 having been EOL'd in 2020, it still accounts for more downloads than 3.10 (according to pypistats) Using an ordered dict would solve (3), and allow using an LRU rather than a FIFO, but it would not actually prevent double-pops or double-inserts, that would require a proper lock on lookup. Which might not be that expensive but given the lack of a good dataset to bench with, it seems a lot of additional complexity for something we've got no visibility on. But that can be considered if someone reports a serious performance regression from this. So for now just revert to a "reset" cache replacement policy. If / when we drop older versions we can switch to `functools.lru_cache` and let the stdlib take care of this (and possibly have cache stats). Alternatively if we get a good testing dataset one day we can bench cache replacement policies or even provide pluggable policies. Anyway fixes ua-parser#132, closes ua-parser#133

Amongst other changes, #113 switched the cache to a FIFO inspired by the standard library's re module, however it didn't really take concurrency in account, so didn't really consider: that double-pops are possible (probably why the stdlib ignores a bunch of errors), which can cause KeyError during lookup (as two workers try to clear the first key, one succeeds, and the other doesn't find the key and fails). It also has a few other less major issues: - double-inserts are possible, which can cause the cache to exceed set capacity permanently by the number of concurrent workers - the stdlib's method only works properly with Python 3.6's naturally ordered `dict`, but I'd rather not drop 2.7 compatibility from 0.x unless there are very good causes to as, despite 2.7 having been EOL'd in 2020, it still accounts for more downloads than 3.10 (according to pypistats) Using an ordered dict would solve (3), and allow using an LRU rather than a FIFO, but it would not actually prevent double-pops or double-inserts, that would require a proper lock on lookup. Which might not be that expensive but given the lack of a good dataset to bench with, it seems a lot of additional complexity for something we've got no visibility on. But that can be considered if someone reports a serious performance regression from this. So for now just revert to a "reset" cache replacement policy. If / when we drop older versions we can switch to `functools.lru_cache` and let the stdlib take care of this (and possibly have cache stats). Alternatively if we get a good testing dataset one day we can bench cache replacement policies or even provide pluggable policies. Anyway fixes #132, closes #133

masklinn force-pushed the extreme-caching branch from e3f8141 to 60e585d Compare May 1, 2022 17:34

masklinn merged commit a32a59e into ua-parser:master May 1, 2022

masklinn deleted the extreme-caching branch May 1, 2022 17:47

masklinn mentioned this pull request Aug 27, 2022

Revert cache FIFO replacement policy to reset/clear #134

Merged

masklinn mentioned this pull request Sep 24, 2022

Use better cache? #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve caching #113

Improve caching #113

masklinn commented May 1, 2022

Improve caching #113

Improve caching #113

Conversation

masklinn commented May 1, 2022

Unified Cache

Cache Policy