[PATCH 5 of 6 V2] obscache: instantiate the cache and keep it warm

[Pierre-Yves David]

(I unified the two emails to keep the discussion simpler)

On 05/23/2017 06:16 AM, Jun Wu wrote:
> Excerpts from Pierre-Yves David's message of 2017-05-20 17:30:19 +0200:
>> [...]
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -1272,6 +1272,9 @@ class localrepository(object):
>>              self.ui.debug('updating the branch cache\n')
>>              branchmap.updatecache(self.filtered('served'))
>>
>> +        self.obsstore.obscache.update(self)
>> +        self.obsstore.obscache.save(self)
>
> It seems the cache will not be saved for read operations, but is always
> rebuilt when accessed.

This is correct for the state achieved after this series. I left out 
smarter logic in this area to keep it simpler (see comment in patch 6). 
There will be quick follow up to address that.

> That means, without a cache, hg commands like "hg id" at Facebook will take
> 9 seconds no matter how many times you run it (as long as no write operation
> is executed).
>
> I think it might be better to just fallback to the old code path if the
> cache does not exist.

This is exactly what the follow up series will be about. We are using 
this approach in the evolve extensions for a while. If I'm not mistaken 
you are likely shipping that code to your users for a couple of weeks.

Associated code in evolve:

 
https://www.mercurial-scm.org/repo/evolve/file/tip/hgext3rd/evolve/obscache.py#l464

Relevant comment in patch 6:

 >> +    # XXX There are a couple of case where the cache could not be 
up to date:
 >> +    #
 >> +    # 1) no transaction happened in the repository since the upgrade,
 >> +    # 2) both old and new client touches that repository
 >> +    #
 >> +    # recomputing the whole cache in these case is a bit slower 
that using the
 >> +    # good old version (parsing markers and checking them). We 
could add some
 >> +    # logic to fall back to the old way in these cases.
 >> +    obscache = repo.obsstore.obscache
 >> +    obscache.update(repo) # ensure it is up to date:
 >> +    isobs = obscache.get
 >> +
 >> +    # actually compute the obsolete set

> When thinking about it more, the only performance difference with the index
> approach is the overhead converting between revs and nodes. It seems
> possible to control that overhead to about 20ms for 6k revisions.

The node lookup in the indexes is also going to be slower than the 
bytearray indexing. So I expect some extra overhead here too.

> If the performance difference is small (like 20ms), I wonder if it's still
> worthwhile to go the obscache approach since it's not that simple and the
> initial cache build time is not very pleasant for huge repos. That could be
> a perf hit for automations which reclones.

It seems to be still worthwhile. 20ms is still about x20 slower than the 
order of magnitude we are talking about. (+ lookup cost)

(For other reader, here is the current timing record and analysis:
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/098348.html)

There are also couple of easy thing we can do to avoid full cache 
invalidation.

On 05/23/2017 04:22 AM, Jun Wu wrote:
> I have got a chance to test this in production with one of our biggest repo
> and a 18MB obsstore. I tried running "hg id" with a a minimal hgrc (with
> just lz4revlog and remotefilelog enabled) and without an existing obscache.
>
> dualsourcecache.update takes 9 seconds. 7 seconds are used to scan
> changelog, 0.36 seconds used to scan obsstore.
>
> The related profiling data is as below:
>
>   9735       | _computeobsoleteset              obsolete.py:1554
>   9421        \ update                          obsolete.py:1227
>   1872          \ _upgradeneeded                obsolete.py:1311
>    434            \ revs (many times)           changelog.py:319
>   1179            \ __iter__                    obsolete.py:564
>   1179             | __get__                    util.py:798
>   1177             | _all                       obsolete.py:707
>   1138             | _readmarkers               obsolete.py:444
>   1138             | _fm1readmarkers            obsolete.py:432
>   7549          \ _updatefrom                   obsolete.py:1433
> * 7185            \ _updaterevs                 obsolete.py:1439
>    766              \ __get__                   util.py:798
>    766               | successors               obsolete.py:717
>    766               | _addsuccessors           obsolete.py:506
> * 3964              \ node (many times)         changelog.py:359
>    364            \ _updatemarkers              obsolete.py:1471
>
> So it is expensive for us to rebuild the cache. It seems the cache may be
> dropped in some cases (ex. histedit --abort). I wonder if we can make the
> cache more persistent so it'll be impossible to be nuked entirely, like when
> doing a strip, just truncate the cache file to the strip point instead of
> nuking it.

The simplest solution I can see here is to backup the cache when 
histedit/rebase starts and reinstall the cache when the operation is 
aborted. That way we can incrementally upgrade from that backup point. 
This approach can be generalized to other caches suffering from strip.

Stripping the cache is also an option, but only in the case where only 
the changelog has been stripped (not the obsstore).

(note: there are likely simple optimization lurking in update function 
too. For example if len(obsstore) << len(repo), we could focus on 
iterating on the obsstore only when rebuilding the cache, etc…).



Does these details address your concerns?

Cheers.

-- 
Pierre-Yves David