Code review comment for lp:~gz/bzr/lru_cache_refactor

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/14/2011 10:12 PM, Martin Packman wrote:
> Martin Packman has proposed merging lp:~gz/bzr/lru_cache_refactor
> into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~gz/bzr/lru_cache_refactor/+merge/79439
>
> Remove some complications from bzrlib.lru_cache that made it harder
> for me to understand. There should be no functional change for
> Bazaar.
>
> The LRUCache and LRUSize cache objects basically act like dicts,
> but may also evict items if a threshold is passed, starting with
> those touched 'least recently'.
>
>
> The branch addresses the following wrinkles:
>
> * Deprecates the `items` method that doesn't return a list of
> 2-tuples. This seems to be mostly for test usage so has been
> renamed to `as_dict` which is what it actually does.
>
> * There's a test helper method `walk_lru` that may as well live
> with the tests.
>
> * Given that usage of LRUSizeCache in bzrlib sensibly sticks to
> using to store simple immutable (or sort of at least,
> GroupCompressBlock is a bit odd) objects, their size may as well be
> recalculated on eviction. This is a judgement call, given bad
> objects that change size this could make caches behave oddly.
> That's already the case if given a `_compute_size` function that
> returns funny values.

If something's size does change, then this quickly messes up the size
tracking. I think it is reasonable to eject an item with the size that
you added it.

>
> * Remove per-node callback functions. Given that bzrlib has no
> callers that want this, and to work properly they need a fair bit
> of fixing, this seems like the simplest option. In short, the
> issues are:
>

I do believe there is already 3rd party code (Launchpad, loggerhead?)
that makes use of this. bzr-cvsps-import I think also uses it.

John
=:->

> ** The nomenclature is confusing. The caches have a cleanup method,
> and their nodes have a cleanup slot that calling cleanup may, or
> may not, call.
>
> ** LRUSizeCache.add doesn't do the right thing with cleanup
> callbacks. It uses the one passed in with the new node if an old
> node is being replaced and there's not room for the new node,
> rather than the function from the slot. Otherwise when replacing a
> node, it doesn't call cleanup on the old node at all.
>
> ** The cleanup is per-node, but even the tests for it use it for a
> per-cache callback only.

bzr-cvsps-import passes in Branch objects, and uses their
Branch.unlock() as the cleanup callback. Which does use a per-node
callback (every branch's unlock is different). We can certainly update
the tests to reflect this.

>
> ** It'd be risky anyway to put things that need explict finalizing
> in a cache like this which may keep them alive indefinitely.
>
> ** The `add` method that exposed the cleanup object has been
> deprecated in favour of just using the dict-style access that
> nearly all callers use anyway. This avoids mixing up the check for
> removed functionality with live code.
>
> ** It seems that bzrlib.fifo_cache has a similar cleanup callback
> interface, but if it's not being used for lru_cache I see no reason
> to keep it around.
>
> ** I tried having a test-only cache with a cleanup callback, but
> basically the tests that need it are only testing things to do with
> the callback, the other tests do well enough at ensuring the caches
> behave in a sane manner.
>

bzr-cvsps-import isn't particularly active, so I'd be moderately ok
breaking it. But the code was there for a reason.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6ZX+YACgkQJdeBCYSNAANFrgCguMv0Hw6PaLjd7Kqc5XIMx1Ev
ZkcAn1RkmRGs77mk1zXDjebE1f5rwrp/
=6Ut5
-----END PGP SIGNATURE-----