[PATCH] obsolete: introduce caches for all meaningful sets

[Pierre-Yves David]

Than would be very nice if people stop quoting the whole hundreds lines patches to drop a few comments. Please reduce your quoting to the part you are talking about. I missed your first comment in my initial reading :-/

On 28 août 2012, at 16:13, Idan Kamara wrote:
On Mon, Aug 27, 2012 at 7:30 PM, <pierre-yves.david at logilab.fr> wrote:
> > +# mapping of 'set-name' -> <function to computer this set>
> > +cachefuncs = {}
> > +def cachefor(name):
> > +    """Decorator to register a function as computing the cache for a
> > set"""
> > +    def decorator(func):
> > +        assert name not in cachefuncs
> > +        cachefuncs[name] = func
> > +        return func
> > +    return decorator
> > +
> > +def getobscache(repo, name):
> > +    """Return the set of revision that belong to the <name> set
> > +
> > +    Such access may compute the set and cache it for future use"""
> > +    if not repo.obsstore:
> > +        return ()
> > +    if name not in repo.obsstore.caches:
> > +        repo.obsstore.caches[name] = cachefuncs[name](repo)
> > +    return repo.obsstore.caches[name]
> > +
> > +# To be simple we need to invalidate obsolescence cache when:
> > +#
> > +# - new changeset is added:
> > +# - public phase is changed
> > +# - obsolescence marker are added
> > +# - strip is used a repo
> > +def clearobscaches(repo):
> > +    """Remove all obsolescence related cache from a repo
> > +
> > +    This remove all cache in obsstore is the obsstore already exist on
> > the
> > +    repo.
> > +
> > +    (We could be smarter here given the exact event that trigger the
> > cache
> > +    clearing)"""
> > +    # only clear cache is there is obsstore data in this repo
> > +    if 'obsstore' in repo._filecache:
> > +        repo.obsstore.caches.clear()
> 
> Not sure I understand why you're looking into the filecache here.

repo.obsstore trigger the parsing of all obsolescence markers.

If obsstore is not loaded yet we do not need to invalidate anycache.


> > +
> > + at cachefor('obsolete')
> > +def _computeobsoleteset(repo):
> > +    """the set of obsolete revisions"""
> > +    obs = set()
> > +    nm = repo.changelog.nodemap
> > +    for prec in repo.obsstore.precursors:
> > +        rev = nm.get(prec)
> > +        if rev is not None:
> > +            obs.add(rev)
> > +    return set(repo.revs('%ld - public()', obs))
> > +
> > + at cachefor('unstable')
> > +def _computeunstableset(repo):
> > +    """the set of non obsolete revisions with obsolete parents"""
> > +    return set(repo.revs('(obsolete()::) - obsolete()'))
> > +
> > + at cachefor('suspended')
> > +def _computesuspendedset(repo):
> > +    """the set of obsolete parents with non obsolete descendants"""
> > +    return set(repo.revs('obsolete() and obsolete()::unstable()'))
> > +
> > + at cachefor('extinct')
> > +def _computeextinctset(repo):
> > +    """the set of obsolete parents without non obsolete descendants"""
> > +    return set(repo.revs('obsolete() - obsolete()::unstable()'))
> 
> It seems to me like you reinvented the wheel here with
> this cachefor decorator since we already have a mechanism
> for caching expensive stuff.
> 
> Why aren't these methods properties of some class and decorated
> with propertycache?

These cache need a repository to be computed and obsstore does not have access to a repository. An not eager to add these cache to the repository itself which is already big enough. Moreover the cache data are really related to the content of obsstore. Having the cache on obsstore allows `obsstore.add` method to clear the cache.

In addition we have a lot of sets to cache. We now have "obsolete", "unstable", "extinct" later expect and least two more "latecomer" and "conflicting" and maybe some other useful cache that may emerge. This really looks like entry in a dictionary more than in N different attributes to an object.

These sets need frequent invalidation and purging N propertycache is far more convoluted than a simple call to caches.clear().

The current approach is mostly similar to the one used for the phases caches.

--

The @cachefor decorators may be a bit overkill and I could assign the function to the dictionnary directly.

cachefuncs = {
  'obsolete': _computeobsolete,
  …
}

I used this decorator construct in the experimental extension to ease progressive introduction of various set. I prefer it but won't argue too much about it.

-- 
Pierre-Yves