[PATCH 1 of 6 V2] obsstore: add a 'cachekey' method
Jun Wu
quark at fb.com
Mon Jun 5 02:42:09 EDT 2017
Excerpts from Pierre-Yves David's message of 2017-06-04 04:50:22 +0200:
> [...]
>
> That get me to think a bit more about that part, trying to pin point the
> apparent misunderstanding about the discussion around this series. And I
> think I got it :-)
>
> Overall this series adds:
>
> 1) a generic cache key for the obsstore, to be used by any caches
>
> 2) an abstract class using the above key to build any kind of caches
> that just need to implement a "update yourself with these new markers" +
> storage. (That class is currently combined with a changelog related key,
> see below)
>
> 3) a concrete cache using the above(bytearray to cache a small property)
>
> 4) all the logic around transaction to have that cache updated and
> flushed properly.
>
>
> In the above list, points (1), (2) and (3) are -also- needed by the
> indexes cache and would help its implementation. Dropping the obscache
I'm not sure how did you get that conclusion. I didn't use any of them
naturally.
> in favor of the indexes means dropping a small part of the above series
> only, the rest can be kept around (or updated in place to access a
> different cache with the same API). This highlight that the two series
> are not competing, they are complementary.
>
> The incremental update and race condition prevention logic is quite
> important and the version in this series has been battle tested for a month.
I think the race condition problem is created by the unnecessary complexity
of dual source. The index has a single append-only source and it uses
atomictemp so its handling is clean and easy to be verified.
> In addition, the version of the abstract class in this series is kept
> simple for clarity purposes. However there are many improvement to be
> made to it. Some are already implemented in the evolve extension, some
> needs works into core (I've seen some of the needed bits done by Jun in
> his RFC). Such improvement will benefit all obsstore related cache
> (including the one in the evolve extension). I've been looking forward
> to implement such improvement for a couple of weeks now :-/
The abstraction might be useful for other purposes. But I'm not convinced
that they are necessary for now. Maybe I will change my mind when I start
thinking about marker exchange.
> ---
>
> About the changelog bit in "dualsourcecache". The abstract class use a
> key checking both the changelog (like branchmap, branchrev, tagnode,
> etc) and the obsstore. It does so because my current two concrete cache
> (obscache and obshashrange-cache) need use these two items. We could
> easily split the obsstore part out for cache that rely on the obsstore
> content only (eg: the jun radix tree). Having that logic right and
> easily reusable it a critical point to make progesss on evolution. If
> that help I can follow up making that logic usable outside of
> dualsourcecache. That would let me work on improving that abstract base
> class and allow indexes to reuse it, making that work easier.
I cannot foresee why I want to use "dualsourcecache" in indexing.
> Cheers,
>
> >[…]
>
More information about the Mercurial-devel
mailing list