[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