[PATCH 5 of 8] obscache: add a cache for 1/2 of the "obsolete" property

[Pierre-Yves David]

On 05/20/2017 12:32 AM, Augie Fackler wrote:
>> On May 19, 2017, at 15:15, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>>
>> On 05/19/2017 11:53 PM, Augie Fackler wrote:
>>> On Fri, May 19, 2017 at 04:28:04PM +0200, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david at octobus.net>
>>>> # Date 1495197986 -7200
>>>> #      Fri May 19 14:46:26 2017 +0200
>>>> # Node ID c2b1c81b3c4cba8cc21b1a1b92f4e34e6aa1807a
>>>> # Parent  b5c984cd0640255b94f22d56a4bfe398b2c5de2e
>>>> # EXP-Topic obscache
>>>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r c2b1c81b3c4c
>>>> obscache: add a cache for 1/2 of the "obsolete" property
>>>
>>> So, can you point me to other (upcoming) concrete subclasses of
>>> dualsourcecache, or is this the only one?
>>
>> There are current a second user of this abstract class in evolve. It is used for obsdiscovery[1]. And I expect to we'll experiment with more of this class of cache in the near future.
>>
>> [1] https://www.mercurial-scm.org/repo/evolve/file/71d242e0fc1a/hgext3rd/evolve/obsdiscovery.py#l440
>>
>> The logic in the cache is easy to screw up and the end result is really need to use (implement save/load/reset/update(revs, obs)). Having a class ready to use is -really- handy.
>
> I wish it could be done without the use of inheritance, but it's fine-ish for now as an abstract class. Should it perhaps be using the abc module to be a more explicit base class?
>
> (I haven't ever used abc, so "no" or "not now" is fine)

That looks like a good idea, sold!

>> In addition, there are final stage of skip-obsstore optimization that requires some rework and collaboration in core something hard to access from an extension.
>>
>> So, having this intermediate abstract class is a corner-stone of the current contribution. (most of work in evolve 6.2 is actually about extracting and reusing this logic)
>>
>>> Also, could we put these big new classes in obscache.py or some other
>>> new file? I'm wary of adding more code to the already-1200-lines
>>> obsolete.py, and since these are coming from evolve I'm hoping that
>>> they're still easily separable from obsolete.py...
>>
>> This crossed my mind when adding it. However, there are significant improvement to that class arriving soon. These improvement will need to access (and refactor) some lower logic around reading markers and al. I think it is wiser to wait for the dust to settle after these upgrade and cut where this is
>
> I'm not really satisfied with this answer. Can you split things up into a better organization scheme so we don't end up with more of a mess in this already far-too-large module? That'd make things easier for me to reason about.

Since, I did not used the "not now" voucher for the question above. I 
would like to it here :-). That module can be split up, but not now.

This series is about improving caching. It lands new cache related code 
right next to other cache related code. The module grow bigger but not 
messier.

Splitting this module (likely by creating a obsutil.py module for 
utility functions) will require proper work (redirecting internal user, 
adding deprecation for external user of the util, etc). That is a 
significant unrelated scope bloat that is unlikely to affect the review 
of this series.

There are nothing urgent in splitting this module, it is big, but not 
massive by our current standard (there are about25 modules larger). I've 
already split and cleaned many modules so you can trust me to keep a 
good balance here.

I'll send a V2 to make the discussion move forward on the code.

>> (the version in evolve access some "_private" method and duplicates some code)
>
> ^ this is a sign to me that some more refactoring should probably happen before this lands, as much as I want it now.

I just see it as a sign there are advance experiments going on in the 
evolve extension.

Cheers,

-- 
Pierre-Yves David